Skip to content
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

Implement clearer syntax for deep loading #1018

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

aeplay
Copy link
Contributor

@aeplay aeplay commented Dec 13, 2024

No description provided.

Copy link

vercel bot commented Dec 13, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
form-demo ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 16, 2024 3:58pm
gcmp-homepage ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 16, 2024 3:58pm
jazz-chat-1 ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 16, 2024 3:58pm
jazz-chat-2 ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 16, 2024 3:58pm
jazz-homepage ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 16, 2024 3:58pm
11 Skipped Deployments
Name Status Preview Comments Updated (UTC)
books-demo ⬜️ Skipped (Inspect) Dec 16, 2024 3:58pm
clerk-demo ⬜️ Skipped (Inspect) Dec 16, 2024 3:58pm
image-upload-demo ⬜️ Skipped (Inspect) Dec 16, 2024 3:58pm
jazz-chat ⬜️ Skipped (Inspect) Dec 16, 2024 3:58pm
jazz-inspector ⬜️ Skipped (Inspect) Dec 16, 2024 3:58pm
jazz-pets ⬜️ Skipped (Inspect) Dec 16, 2024 3:58pm
jazz-todo ⬜️ Skipped (Inspect) Dec 16, 2024 3:58pm
music-demo ⬜️ Skipped (Inspect) Dec 16, 2024 3:58pm
passkey-demo ⬜️ Skipped (Inspect) Dec 16, 2024 3:58pm
passwords-demo ⬜️ Skipped (Inspect) Dec 16, 2024 3:58pm
reactions-demo ⬜️ Skipped (Inspect) Dec 16, 2024 3:58pm

@aeplay aeplay force-pushed the jazz-581-rfc-new-deep-loading-api branch from 6c39be0 to 6a8fa16 Compare December 16, 2024 15:56
@vercel vercel bot temporarily deployed to Preview – passkey-demo December 16, 2024 15:57 Inactive
@vercel vercel bot temporarily deployed to Preview – books-demo December 16, 2024 15:57 Inactive
@vercel vercel bot temporarily deployed to Preview – clerk-demo December 16, 2024 15:57 Inactive
@vercel vercel bot temporarily deployed to Preview – image-upload-demo December 16, 2024 15:57 Inactive
@vercel vercel bot temporarily deployed to Preview – reactions-demo December 16, 2024 15:57 Inactive
@vercel vercel bot temporarily deployed to Preview – jazz-todo December 16, 2024 15:57 Inactive
@vercel vercel bot temporarily deployed to Preview – jazz-chat December 16, 2024 15:57 Inactive
@vercel vercel bot temporarily deployed to Preview – jazz-inspector December 16, 2024 15:57 Inactive
@vercel vercel bot temporarily deployed to Preview – passwords-demo December 16, 2024 15:57 Inactive
@vercel vercel bot temporarily deployed to Preview – music-demo December 16, 2024 15:57 Inactive
@vercel vercel bot temporarily deployed to Preview – jazz-pets December 16, 2024 15:57 Inactive
): { me: Acc | DeeplyLoaded<Acc, D> | undefined; logOut: () => void } {
function useAccount<const O extends { resolve?: RefsToResolve<Acc> }>(
options: O,
): { me: Resolved<Acc, O> | undefined; logOut: () => void };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This kind of typing makes ok to add new options like:

Screenshot 2024-12-17 at 16 17 56

Do you think it would be ok to have just:

useAccount<const R extends RefsToResolve<Acc>>(
    options: { resolve?: R },
  ): { me: Resolved<Acc, R> | undefined; logOut: () => void };

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also noticed that with this change it would be possible to add docs on the resolve option:
Screenshot 2024-12-17 at 16 23 34

Copy link
Contributor

@gdorsi gdorsi Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed two other issues:

  1. Additional properties added inside to resolve aren't marked as invalid
  2. Errors on the value side are quite cryptic

We probably can't do much to make errors easier to read, but we should definitely mark additional properties as errors.

See:
Screenshot 2024-12-17 at 16 42 13

Screenshot 2024-12-17 at 16 38 31

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants