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

feat: Auto-typing support for entries #1983

Merged
merged 5 commits into from Apr 28, 2023

Conversation

tcc-sejohnson
Copy link
Contributor

See sveltejs/kit#9571. This adds auto-typing support for that. I think?

The behavior should be:

  • entries is a function returning Array<RouteParam>
  • entries can be returned from any Kit file except Layout files

But I can't seem to get the host to work locally, so I'm not sure if this works or not... 👀

? entries.node.equalsGreaterThanToken.getStart()
: entries.node.body.getStart();
const returnInsertion = surround(
`: Promise<Array<import('./$types.js').RouteParams>> | Array<import('./$types.js').RouteParams> `
Copy link
Member

Choose a reason for hiding this comment

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

In the related PR I see EntryGenerator being generated, shouldn't we use that one instead?

Also, is RouteParams the right one? And in the PR it says it's also valid for +server.js - does that have any implications on this?

Copy link
Member

Choose a reason for hiding this comment

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

yeah, RouteParams seems right to me — this can't be used with LayoutParams.

Using EntryGenerator is probably better but does that mean we can't release this until sveltejs/kit#9571 is released? because i was thinking we couldn't release sveltejs/kit#9571 until this is released 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. RouteParams is correct
  2. Can we use EntryGenerator? How would we apply that type if it's a function entries declaration instead of a const entries declaration?
  3. For other exports like load, I see JSDoc annotations with links to the docs. I don't think I'm adding that here. How do those work?

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to use EntryGenerator because if something changes in the API we don't need to adjust it here (I thought about offloading some of this stuff in here into SvelteKit to not have the chicken/egg problem in the future, but that's a different story). I don't see a problem with "which comes first" because we can still release this first - it just will resolve to nothing until you have a route with params and the minimum required SvelteKit version. We can do something like ReturnType<EntryGenerator> if EntryGenerator is the whole thing, not just the return type. JSdoc works because while syntactically invalid TS still parses the types when annotated as TS types 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, think we should be good to go, but please help me make sure those angle brackets are matching correctly. my eyes are crossing. 🤣

Copy link
Member

Choose a reason for hiding this comment

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

In the related PR EntryGenerator is typed as export type EntryGenerator = () => Promise<Array<RouteParams>> | Array<RouteParams>; - that means we should be good with just : ReturnType<import('./$types').EntryGenerator>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do astound myself with my own stupidity from time to time 🤣 Implemented.

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

🎉

@dummdidumm dummdidumm merged commit 651db67 into sveltejs:master Apr 28, 2023
2 checks passed
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.

None yet

3 participants