Factored out graph operations and moved some code into core #83

riccardoferretti · 2020-07-09T21:55:30Z

I will comment in-line to guide the reasoning

packages/foam-vscode/src/workspace.ts

packages/foam-workspace-manager/src/core.ts

packages/foam-workspace-manager/src/utils/utils.ts

riccardoferretti · 2020-07-09T22:09:30Z

packages/foam-vscode/src/extension.ts

-  //     `[backlink:${backlink.id}]: ${backlink.filename} "${backlink.title}"`
-  //   );
-  // }
+  const references = uniq(createMarkdownReferences(foam, id)


now the only thing the extension does is converting the md references for the editor (looking at it even the string creation should go in the core)

This is somehow related to the link reference definition proposal, currently I'm not quite sure in which order it would be beneficial to implement these things

yes tbh I am not fully clear on how the link reference would work best, in this case I was reimplementing the existing behavior, but I agree this needs a bit more clarity

@riccardoferretti is the purpose of uniq here to dedup the list in cases where there were multiple links to the same document? Unless those references are triple equal to each other, this will still leave duplicates as lodash#uniq uses SameValueZero comparison, which for object compares reference equality, not deep equality.

yeah, that's why I am doing the uniq on the resulting string, which is kinda of a hack, this should be done at the createMarkdownReferences layer (on the object as you hinted at), I was a bit lazy about doing the right comparison. I should really move it there..

jojanaho · 2020-07-10T21:17:26Z

packages/foam-workspace-manager/package.json

    "@types/lodash": "^4.14.157",
+    "graphlib": "^2.1.8",


Is it possible to store metadata about nodes & edges with this library or should they live in a separate index? E.g. links should probably contain e.g. line and column numbers + text length to support proper highlighting in the UI.

yes good point - it's possible to add metadata to node/edges. I am currently storing the Bubble object in the node, and just the text of the link in the edge (but I should probably store an object there instead, to more easily prepare for extra metadata)

jevakallio

@riccardoferretti I noticed you've just pushed a new commit while I was reviewing, so let me conclude my initial review pass here and start a new one with the new changes.

packages/foam-workspace-manager/src/core.ts

jevakallio · 2020-07-11T07:44:35Z

packages/foam-workspace-manager/src/core.ts

+
+  public setBubble(bubble: Bubble) {
+    if (this.graph.hasNode(bubble.id)) {
+      (this.graph.outEdges(bubble.id) || []).forEach(edge => {


It's not clear to me yet why this is done, but maybe it'll become clear later

basically this cleans the old edges before adding the new ones, otherwise the old links would still exist - I might actually move that out and have a removeBubble (which we would need when removing a file anyways)

packages/foam-workspace-manager/src/utils/utils.ts

jevakallio · 2020-07-11T08:18:07Z

packages/foam-workspace-manager/src/core.ts

+    })
+  }
+
+  public getBubbles(): Bubble[] {


Whether we call this Bubble.getBubbles, or Note.getNotes, is that naming clear? Should it be .getLinkedNotes?

Not sure tbh, thinking out loud here.

This one returns all the notes in the graph, whether or not they are linked to others, that's why I kept the name bare, what do you think?

Somewhat related, I tend to use the following conventions:

getNote(id): Note expects the id to exist and will throw if it doesn't find it

findNote(id): Note | null for those situations when it's unsure whether the id exists, but don't wanna fail (alternative would be to have hasNote(id), I slightly prefer the find syntax)

listNotes(): Note[] when dealing with arrays

If you like them we could also use those conventions, I am super easy

I'm fine with the current convention in the latest iteration of this PR 👍

jevakallio · 2020-07-11T10:06:08Z

packages/foam-workspace-manager/src/core.ts

+  public path: string
+  public links: OutLink[]
+
+  constructor(id: ID, title: string, links: OutLink[], path: string, source: string) {


Personally, for long argument lists made mainly of primitive data types, I prefer a named input object instead of positional arguments, since it's too easy to accidentally e.g. swap path and source order at call site

I agree 100%.

In fact, for Note, I was thinking of just making it a type/interface, as I have removed all the methods I initially added here

@riccardoferretti yes, all for using types over classes whenever we can!

jevakallio · 2020-07-11T10:11:45Z

packages/foam-vscode/src/extension.ts

-  //     `[backlink:${backlink.id}]: ${backlink.filename} "${backlink.title}"`
-  //   );
-  // }
+  const references = uniq(createMarkdownReferences(foam, id)


@riccardoferretti is the purpose of uniq here to dedup the list in cases where there were multiple links to the same document? Unless those references are triple equal to each other, this will still leave duplicates as lodash#uniq uses SameValueZero comparison, which for object compares reference equality, not deep equality.

jojanaho · 2020-07-11T20:22:54Z

packages/foam-cli/src/commands/hello.ts

@@ -23,7 +23,7 @@ hello world from ./src/hello.ts!
    const {args, flags} = this.parse(Hello)
    const name = flags.name ?? 'world'

-    const wm = new WorkspaceManager('./foo');
+    const wm = new NoteGraph();


would make sense to call this "ng" - or maybe just "g" :)

Or, gosh, even graph to be painfully explicit :)

(This is in the dummy code in CLI, however, so not mega important, since this file won't live through first real commit to the CLI project)

good point, here I was just fixing the object used, wasn't paying attention to the name of the variable

as it's just a dummy I will leave it that way

nevermind, there was a compilation error afterwards, so I just commented this part of the function :)

jevakallio · 2020-07-11T20:30:29Z

docs/meeting-notes/foam-core-2020-07-11.md

+- `ast`: raw markdown ast
+- `checksum`: if we do caching
+
+### Link text


Reviewers, please ignore this, needs cleaning up -- committed working scratchpad here accidentally

jevakallio · 2020-07-11T20:35:49Z

packages/foam-cli/package.json

@@ -1,5 +1,5 @@
 {
-  "name": "foam-cli",
+  "name": "@foam/cli",


Is the namespace/scope required for the TypeScript scoping? We don't own the @foam scope on npm so we can't publish the CLI by this name.

good point I didn't think of that. will use foam-core, foam-cli and foam-vscode

jojanaho · 2020-07-11T20:56:08Z

docs/meeting-notes/foam-core-2020-07-11.md

+### Open questions
+
+- How should writing to files work
+  - What if affected notes have unsaved changes


Good question! Writing to files (in the context of VS Code) should probably be done via something like applyEdit API (search from https://code.visualstudio.com/api/references/vscode-api#workspace)

docs/meeting-notes/foam-core-2020-07-11.md

jojanaho · 2020-07-11T21:03:17Z

docs/meeting-notes/foam-core-2020-07-11.md

+
+### Node definition
+
+What do we need the node (and edge metadata) to contain:


How about the graph metadata, e.g. "root folder of the graph"?

As per comment later on, adding this would not hurt, so that implementers will have access to it if needed

jojanaho · 2020-07-11T21:03:56Z

docs/meeting-notes/foam-core-2020-07-11.md

+What do we need the node (and edge metadata) to contain:
+
+- `id`: tbd
+  - should be unique, needs some kind of unique gen function


Would path in relation to the graph root folder work as unique id?

@jojanaho#3766 these are my scribbles, I pushed these to @riccardoferretti's branch while we were hacking on a call

File path relative to workspace root is exactly what we landed on.

I started to look into this, there are a couple of open questions that we can chat about. I will not include it in this PR though so we can merge it in the meantime

jojanaho · 2020-07-11T21:04:55Z

docs/meeting-notes/foam-core-2020-07-11.md

+- `id`: tbd
+  - should be unique, needs some kind of unique gen function
+  - should be reconstructable even if links are not updated every time
+  - what happens during rename? is reparenting the graph going to be hard?


What does the reparenting of the graph mean in this context?

I believe my thinking here was that if we use file path as id, and the file's path changes, is it possible to update the node's id, or must the node be deleted, new Note node be created, and then all in/outEdges be re-routed to it.

If id is mutable, then this is no issue.

If it's not, it's merely an inconvenience.

I was leaning towards having the ID being the URI of the file, basically case n.2
I don't see it as a big issue, and I think it might be cleaner, but I am open to the idea of having another ID for the note (the question would be how to generate it/store it/....)

jojanaho · 2020-07-11T21:05:57Z

docs/meeting-notes/foam-core-2020-07-11.md

+  - what happens during rename? is reparenting the graph going to be hard?
+  - do id's need to be persistent, or can we create them per in-memory session, keep them stable despite renames, and then next session generate a new id?
+  - Ideally should be a path to file, so it's easy to look up from the graph by id for renaming
+- `type`: Note | Image | etc


Maybe MIME-type?

We could implement a Node TS type that's a union of different types (Note | Image | Etc)

yeah, there are some considerations here similar to the ones I have encountered for the links. we should have a chat about these

jojanaho · 2020-07-11T21:07:48Z

docs/meeting-notes/foam-core-2020-07-11.md

+  - Ideally should be a path to file, so it's easy to look up from the graph by id for renaming
+- `type`: Note | Image | etc
+- `title`: can be read from markdown title or frontmatter metadata
+- `path`: full path to file, relative to workspace (graph) root


The relation of this to id?

The same, if the id is a path, so either we drop path, or we keep both in case we have to change the id function later

I started to look into this, but it won't probably make it in this PR as it turns out a couple of things change around wikilinks as a result

docs/meeting-notes/foam-core-2020-07-11.md

jojanaho · 2020-07-11T21:17:08Z

docs/meeting-notes/foam-core-2020-07-11.md

+
+Not supported by Markdown Notes:
+
+- `[[path/to/file-name.md]]` - Just use markdown links


IMO this is absolutely needed (better yet in format of [[path/to/file-name]]). Foldering of notes would probably be pretty typical use case (is at least to me). It's not particularly hard to implement in comparison to flat notes, and looks like we are already building the base for functionality that's currently in Markdown Notes...

jojanaho · 2020-07-11T21:18:02Z

docs/meeting-notes/foam-core-2020-07-11.md

+
+Issues:
+
+- Name clashes in directories


Not an issue if root-based wikilinks are supported

what do you mean? I am not familiar with root-based wikilinks

@riccardoferretti I think @jojanaho meant the line he commented on above, e.g. [[path/to/file-name.md]].

Sounds like we need to write a further document on aligning how we'll approach this. @jojanaho on our call with Riccardo yesterday we brain stormed a few ideas (in the scratchpad section above) but didn't make any decisions or arrive in firm conclusions

jojanaho · 2020-07-11T21:19:24Z

docs/meeting-notes/foam-core-2020-07-11.md

+Issues:
+
+- Name clashes in directories
+- Name clashes between extensions


This could probably be handled with linting

jojanaho · 2020-07-11T21:21:59Z

docs/meeting-notes/foam-core-2020-07-11.md

+  - Change filename/title needs to reflect everywhere
+- Orphaning
+
+- If we can't rely on in-memory process to rename things correctly while changes happen (e.g. file is renamed, moved, deleted, or titled) <ref id="1" />


Getting this to work properly probably requires using VS code APIs for rename operations (e.g. https://code.visualstudio.com/api/references/vscode-api#RenameProvider)

jojanaho · 2020-07-11T21:23:40Z

docs/meeting-notes/foam-core-2020-07-11.md

+
+How others solve this:
+
+- Unique ids -- could support optionally as part of file name or front matter metadata. Should not be required.


For renames outside VS code file hash could maybe do, inside VS code probably through APIs(?)

@jojanaho could you expand on this?

jojanaho · 2020-07-11T21:25:55Z

packages/foam-core/src/core.ts

@@ -0,0 +1,89 @@
+import { Graph, Edge } from 'graphlib'


Maybe call this file (core.ts) as notegraph.ts?

👍 -- I'd say note-graph.ts since capital G makes it a compound word, and therefore should be separated.

jojanaho · 2020-07-11T21:27:11Z

packages/foam-core/src/core.ts

+export interface NoteLink {
+  to: ID
+  text: string
+}


Maybe add fields from this? https://code.visualstudio.com/api/references/vscode-api#Position

yeah this could be interesting, but I want to better understand the use case (do we need to update the info on every edit, or only on save? depends on what we need the info for I think)

jojanaho · 2020-07-11T21:27:51Z

packages/foam-core/src/core.ts

+  public id: ID
+  public title: string
+  public source: string
+  public path: string


here id vs path

Mentioned this on previous comment, but the fact that the ID happens to be a path is an implementation detail and we shouldn't rely on the fact. If we do need a file path (and we do), I think it would be good to keep it in path

jojanaho · 2020-07-11T21:34:29Z

packages/foam-core/src/core.ts

+  public path: string
+  public links: NoteLink[]
+
+  constructor(id: ID, title: string, links: NoteLink[], path: string, source: string) {


maybe use Partials here? https://stackoverflow.com/a/37682352

I was wondering whether this needs to be a class in the first place, or just a type/interface - what do you think?

jojanaho · 2020-07-11T21:35:41Z

packages/foam-core/src/core.ts

+
+
+export class NoteGraph {
+  private graph: Graph


Should this contain other fields like https://github.com/foambubble/foam/blob/master/packages/foam-workspace-manager/src/WorkspaceManager.ts#L50

Currently the old WorkspaceManager.path was never used, but it may be useful considering that in the graph nodes we want to keep relative urls, and it may be useful to have the information at hand to construct absolute file URIs

yeah I removed it because it wasn't used, and I was debating whether this info should live in the config or the graph. currently we store the absolute paths in the graph, having a base path would allow us to make those relative. why do you prefer relative urls @jevakallio ?

@riccardoferretti that's a good question.

I guess I prefer them for readability -- the paths will be closer to what's being used in links and generated into link ref defs -- but that's not a strong preference.

Another argument for relative paths is that if we ever persist the graph, relative links continue to work when e.g. cloning the repo to different computer, moving it to different folders, or even as logical paths in a GitHub repo / published site.

If there's no good reason for absolute paths in Note.path, I'd rather keep it relative, but can be convinced otherwise.

jevakallio · 2020-07-11T22:02:55Z

docs/meeting-notes/foam-core-2020-07-11.md

+  - `text`: The link label
+  - `type` markdown | mediawiki | image | http
+  - `section`
+  - `block` (ref)


@jojanaho this is in reference to a vague plan I have about building Roam's block references and block embeds into Foam. This is not something we need to think of now, more of a future consideration.

The idea here was to annotate blocks of text (paragraph, bullet, line, etc.) using some method (HTML tags were discussed), and then being able to link to that section from another document such that in VS Code that block is brought as an editable embed to that document. Additionally, this linking would be supported by a hypothetical Foam static site generator, that would be able to construct published pages from blocks in other documents.

jevakallio · 2020-07-11T22:14:34Z

tsconfig.base.json

+    "declarationMap": true,
+    "baseUrl": ".",
+    "paths": {
+      "@foam/core": ["./packages/foam-core/src"],


If the @scope/package naming convention is not mandatory, perhaps we should drop it? Alternatively we'll need another scope, since we don't own the npm user/org @foam. We could do @foambubble, but that makes it slightly confusing as folks might think it's somehow coupled to the GitHub org.

jevakallio

@riccardoferretti did another review, and this is looking better and better on every pass!

I'd like to aim to get a version of foam-core landed tomorrow, as @anku255 and @chirag-singhal may need to start work on the CLI/janitor on Monday, and may need access to this functionality.

I'm ok having some of the details be reworked in future PRs as long as we get the broad strokes correctly:

The public NoteGraph API (some suggestions on file name, constructor signature)
The package naming convention (can't publish to @foam npm org)
Any other issues that we can address in a reasonable time frame.

@riccardoferretti let me know if you don't have time to look at this tomorrow (Sunday) and I can pick up the branch, clean it up and land it

renaming Foam => NoteGraph, Bubble => Note, and related renaming

renamed @foam/core => foam-core and @foam/cli => foam-cli (see #83 (comment))

jevakallio

I tested this and fixed a couple of issues! @jojanaho @riccardoferretti could one of you test this locally and see if you can get it to break? I can't.

If it works for you, feel free to merge, I'm leaving an approving review here on the assumption that my last commits are ok (but don't assume they are).

I'll be back in a few hours.

renamed @foam/core => foam-core and @foam/cli => foam-cli (see foambubble/foam#83 (comment))

riccardoferretti requested review from jevakallio and jojanaho July 9, 2020 21:55

riccardoferretti commented Jul 9, 2020

View reviewed changes

jojanaho reviewed Jul 10, 2020

View reviewed changes

riccardoferretti force-pushed the spike-core-graph branch from a04730f to 9febd6a Compare July 11, 2020 10:08

jevakallio reviewed Jul 11, 2020

View reviewed changes

jevakallio mentioned this pull request Jul 11, 2020

Saving a file generates a new link reference section every time #94

Closed

jojanaho reviewed Jul 11, 2020

View reviewed changes

jevakallio reviewed Jul 11, 2020

View reviewed changes

jojanaho reviewed Jul 11, 2020

View reviewed changes

docs/meeting-notes/foam-core-2020-07-11.md Show resolved Hide resolved

jojanaho reviewed Jul 11, 2020

View reviewed changes

docs/meeting-notes/foam-core-2020-07-11.md Outdated Show resolved Hide resolved

jojanaho reviewed Jul 11, 2020

View reviewed changes

docs/meeting-notes/foam-core-2020-07-11.md Outdated Show resolved Hide resolved

jojanaho reviewed Jul 11, 2020

View reviewed changes

jevakallio reviewed Jul 11, 2020

View reviewed changes

jevakallio mentioned this pull request Jul 11, 2020

Add open daily note base feature #79

Merged

riccardoferretti and others added 9 commits July 12, 2020 02:36

factored out graph operations and moved some code into core

57c34db

minor adjustments

e9150fb

using more sensible names

99d49a0

renaming Foam => NoteGraph, Bubble => Note, and related renaming

foam-core: Meeting notes

d7425f8

extracted foam-core and created @Foam modules

639edaf

update note in graph when saving file

debe948

renamed packages

8bbf972

renamed @foam/core => foam-core and @foam/cli => foam-cli (see #83 (comment))

renamed core.ts => note-graph.ts

e1ac54c

Update docs/meeting-notes/foam-core-2020-07-11.md with notes from PR

5b42385

riccardoferretti force-pushed the spike-core-graph branch from 32345bf to 5b42385 Compare July 12, 2020 00:45

jevakallio mentioned this pull request Jul 12, 2020

Make wikilinks more flexible #93

Closed

jevakallio added 4 commits July 12, 2020 10:37

Formatting: Run prettier

f1424c3

Cleanup: Remove unused imports

df3b657

Don't generate reference headers if file has no wiki links'

d09ccdb

Handle undefined Note nodes gracefully

ab097e1

jevakallio approved these changes Jul 12, 2020

View reviewed changes

riccardoferretti merged commit 3191d7d into master Jul 12, 2020

anku255 mentioned this pull request Jul 12, 2020

Remove references to foam-workspace-manager #100

Merged

Alognas2 added a commit to Alognas2/foam that referenced this pull request Aug 5, 2024

renamed packages

0fda876

renamed @foam/core => foam-core and @foam/cli => foam-cli (see foambubble/foam#83 (comment))

Celoskip3 added a commit to Celoskip3/foam that referenced this pull request Aug 10, 2024

renamed packages

0bb03f5

renamed @foam/core => foam-core and @foam/cli => foam-cli (see foambubble/foam#83 (comment))


		### Node definition

		What do we need the node (and edge metadata) to contain:


		Not supported by Markdown Notes:

		- `[[path/to/file-name.md]]` - Just use markdown links


		How others solve this:

		- Unique ids -- could support optionally as part of file name or front matter metadata. Should not be required.

Factored out graph operations and moved some code into core #83

Factored out graph operations and moved some code into core #83

Conversation

riccardoferretti commented Jul 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jevakallio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jevakallio left a comment

Choose a reason for hiding this comment

jevakallio left a comment

Choose a reason for hiding this comment