-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SET account relation support #206
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor not-important nitpicks. Can be ignored.
await assertSupportedReadModelController(model, commit as unknown as SignedCommitContainer) | ||
}) | ||
await Promise.all( | ||
modelCommitsValues.filter(isSignedCommitContainer).map(async (commit: StreamCommits) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here async and await marks could be omitted. Less troubles for Node.js to unpack the promises.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have more details why it causes troubles for Node please? For V8 using async
/await
is more optimized than hand-written promises.
case 'single': | ||
runtime.accountData[key] = { type: 'node', name: modelName } | ||
break | ||
default: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have UnreachableCaseError
to have exhaustiveness check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I might add this at some point but I'd rather avoid importing more dependencies/APIs from Ceramic if that can be avoided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! The only thing I would say is that I wish you would have done a PR addressing the adaptations to js-ceramic changes first, and then this one.
Thanks! |
There's a bunch of unrelated changes due to the breaking changes in Ceramic APIs, as well as fixing some new linting errors caught by ESLint, but the main relevant changes are:
SET
value as theaccountRelation
in the composite definition schema, as well as theaccountRelationFields
SET
account relation, both for a unique value (with a dedicated input) and all values (similar to theLIST
account relation)SET
account relationset[Model name]
mutations in addition to the now deprecatedcreate[Model name]
forSINGLE
account relations, for consistency with theSET
account relation mutations