-
Notifications
You must be signed in to change notification settings - Fork 152
If a schema is passed, use that for the collection type #186
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
Conversation
🦋 Changeset detectedLatest commit: bb19cac The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Size Change: -130 B (-0.5%) Total Size: 26 kB
ℹ️ View Unchanged
|
|
Size Change: 0 B Total Size: 822 B ℹ️ View Unchanged
|
|
@tanstack/db-example-react-todo commit: |
fd51638 to
975b9d8
Compare
samwillis
left a comment
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.
All looks great!
Aside, with the work on #185 the new useLiveQuery will probably want to optionally take a schema too so that a user can validate writes to a derived collection. I have the groundworks for this.
| value: valueWithKey, | ||
| type: `update`, | ||
| }) | ||
| // Only delete is left as an option |
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.
I think the logic here is correct.
It may be good to add an else that throws with a "this should never happen" error. Currently, if the logic is wrong, the error is silently eaten but we want to know about it.
I quite like the pattern I have seen others using where they have a shouldNeverHappen() function that in dev mode triggers a breakpoint with debugger, and in prod throws an error.
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.
Good idea — I'll add this
| * This type is used internally to resolve the collection item type based on the provided generics and schema. | ||
| * Users should not need to use this type directly, but understanding the priority order helps when defining collections. | ||
| */ | ||
| export type ResolveType< |
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.
Looks good!
You now must either pass an explicit type or schema - passing both will conflict.
I also removed the broken
preloadCollectionso I didn't have to fix its types 😆 the plan is make collections lazy by default and expose a.start()method which returns a promise that resolves when the collection finishes loading.Fix #116 #76