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

Generated types invalid for routes with matched parameter #4462

Closed
wants to merge 2 commits into from

Conversation

istarkov
Copy link
Contributor

@istarkov istarkov commented Mar 29, 2022

Generated types invalid for routes with matched parameter

Ref #4327 #4361

Bug description

The automatically generated types for routes with matched parameters are invalid.
i.e. route like /[lang=is_lang]/[name] will generate following

export type RequestHandler<Output extends ResponseBody = ResponseBody> = GenericRequestHandler<{ lang=is_lang: string; name: string }, Output>;

After this PR

export type RequestHandler<Output extends ResponseBody = ResponseBody> = GenericRequestHandler<{ lang: string; name: string }, Output>;

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

@changeset-bot
Copy link

changeset-bot bot commented Mar 29, 2022

🦋 Changeset detected

Latest commit: bccaa11

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit Patch

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

@benmccann
Copy link
Member

Cool. Thanks for the fix. Perhaps it would be good to expose extract_params and add a unit test for it

@istarkov
Copy link
Contributor Author

Based on this comment #4361 (comment)

We could get the canonical list of params from the manifest data for routes

Seems like implementation will be changed seriously some day, as extracting params using different code doesn't look good.

This is just a quick fix allowing to use modern features.

PS: For me types generated for components like __layout should be changed too, i.e params for __layout should be Record<string, string> instead of {}

@Rich-Harris
Copy link
Member

Thank you. Since write_types was written, a parse_route_id function was added which we can use instead — closing this in favour of #4472

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working types / typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants