ENG-1983 Define intermediate schemas for node, relations and schemas#1183
ENG-1983 Define intermediate schemas for node, relations and schemas#1183maparent wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
3badfb0 to
4bbf2f8
Compare
4bbf2f8 to
a8bc33d
Compare
a8bc33d to
47202b7
Compare
47202b7 to
66ebc78
Compare
| @@ -0,0 +1,92 @@ | |||
| import type { ContentType } from "@repo/content-model"; | |||
There was a problem hiding this comment.
@maparent
Let's only export types that are being used elsewhere. This adds positive signal and really helps navigate and grok the file.
When we export types that are not used, it adds a false positive and makes it harder to understand intent.
sid597
left a comment
There was a problem hiding this comment.
Some questions and suggestions, I read CrossApp... so many time that now I am confused what it even means lol
| sourceApp: "Roam", | ||
| url: ROAM_SOURCE_SPACE_ID, | ||
| }, | ||
| local_id: ROAM_SOURCE_NODE_ID, |
There was a problem hiding this comment.
snake case -> camelCase
What do you think about naming, like appending source vs direct e.g
sourceLocalId, vslocalIdspace.sourceAppvsspace.app
There was a problem hiding this comment.
I have tended to remove source here, because everything was named sourceSomething, and it did not seem to add specific information (as you were remarking about CrossApp!) Maybe I missed something? Fully negotiable.
There was a problem hiding this comment.
+1 to camelCase for TypeScript-facing contract fields
There was a problem hiding this comment.
Agreed. Did the rename in the type and forgot to update the example.
There was a problem hiding this comment.
As for source: With the same reasoning as account, happy to reintroduce it where it fits database names. It was quite broad in the original file, but that argument does not hold anymore. Would you prefer that?
| | { | ||
| localId: string; | ||
| // infer space from context if absent. | ||
| space?: SpaceRef; |
There was a problem hiding this comment.
Is this for future? Can space be absent in Obsidian <-> roam sync?
There was a problem hiding this comment.
This is for the case where we have a reference to a node in another space. It currently happens in Obsidian; the source or destination of a relation may be an imported node, in which case we give the reference to the original node (imported nodes are not materialized again.)
But yes, it can be absent and inferred from context in most cases.
There was a problem hiding this comment.
I'm assuming that localId/space can be used for more use cases? If not, then having it in a union this high up will just complicate things by making the downstream types just more complex for a single use case.
| value: string; | ||
| localId?: string; | ||
| embedding?: CrossAppEmbedding; | ||
| author: CrossAppAccount | Ref; |
There was a problem hiding this comment.
When would author use Ref (specifically LocalRef)?
There was a problem hiding this comment.
This allows a minimal reference to the author, i.e. enough for a reference, not enough for an upsert. I did not check how we currently include authors in the sync, but I think it's closer to Ref usage than to an InlineAccount. Reusing the Ref type is a bit of overloading here, maybe we should define a separate AccountRef. But it would end up having exactly the same parameters.
There was a problem hiding this comment.
I’m still unclear on when author would actually use Ref, specifically LocalRef. Could you give a real world example?
There was a problem hiding this comment.
In convertRoamNodeToFullContent, we use author_local_id which would be the equivalent of a LocalRef.
66ebc78 to
38f9ba0
Compare
885ec68 to
80f3533
Compare
80f3533 to
e4a3d61
Compare
e4a3d61 to
1e9e344
Compare
1e9e344 to
a335e7d
Compare
a335e7d to
768923b
Compare
768923b to
c20936a
Compare
|
Decision: Will be split in sub-tasks. |
fc84691 to
442535f
Compare
442535f to
b42b68f
Compare
b42b68f to
729236e
Compare
729236e to
c9117a2
Compare
c9117a2 to
d3a1f85
Compare
d3a1f85 to
e54938d
Compare
e54938d to
a787c0b
Compare
a787c0b to
8394ac5
Compare
8394ac5 to
859d547
Compare
859d547 to
fab6a5f
Compare
fab6a5f to
f2f5ee7
Compare
https://linear.app/discourse-graphs/issue/ENG-1983/define-intermediate-schemas-for-node-relations-and-schemas