-
-
Notifications
You must be signed in to change notification settings - Fork 196
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
feat: Auto-typing support for entries
#1983
Conversation
? entries.node.equalsGreaterThanToken.getStart() | ||
: entries.node.body.getStart(); | ||
const returnInsertion = surround( | ||
`: Promise<Array<import('./$types.js').RouteParams>> | Array<import('./$types.js').RouteParams> ` |
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.
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?
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.
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 😅
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.
- RouteParams is correct
- Can we use
EntryGenerator
? How would we apply that type if it's afunction entries
declaration instead of aconst entries
declaration? - 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?
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 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 😄
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.
ok, think we should be good to go, but please help me make sure those angle brackets are matching correctly. my eyes are crossing. 🤣
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.
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>
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 do astound myself with my own stupidity from time to time 🤣 Implemented.
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.
🎉
See sveltejs/kit#9571. This adds auto-typing support for that. I think?
The behavior should be:
entries
is a function returningArray<RouteParam>
entries
can be returned from any Kit file exceptLayout
filesBut I can't seem to get the host to work locally, so I'm not sure if this works or not... 👀