Skip to content

fix: allow for whitespace in snippets declaration#2366

Merged
dummdidumm merged 2 commits into
sveltejs:masterfrom
paoloricciuti:better-snippets-generics
May 13, 2024
Merged

fix: allow for whitespace in snippets declaration#2366
dummdidumm merged 2 commits into
sveltejs:masterfrom
paoloricciuti:better-snippets-generics

Conversation

@paoloricciuti

@paoloricciuti paoloricciuti commented May 5, 2024

Copy link
Copy Markdown
Member

This should close sveltejs/svelte#11478

The problem was that the generic generation was relying on the start position of typeAnnotation + 1. But if there are whytespaces this is not correct and the generate type is wrong Snippet<[: string]>. However inside the first typeAnnotation there's always a second typeAnnotation field which only includes the actual types (at least from what i was able to see, i wasn't able to find a type that doesn't generate at least two level deeps typeAnnotation).

This seems to work fine but give it a good look since it's the first time i'm working in this repo.

@paoloricciuti

Copy link
Copy Markdown
Member Author

Umh...i don't know if i've fucked up the expected file here...it seems strange that is a new one.

@jasonlyu123

jasonlyu123 commented May 6, 2024

Copy link
Copy Markdown
Member

This is because the generated snapshot is always written to the v5 file but the expected file can be both. You can move the content to the expetectv2.js file and remove the v5 file. We can tweak the snapshot generation in a separate PR. But It's probably a small change so if you're interested in tweaking it, I wouldn't mind if it's done in this PR.

@paoloricciuti

Copy link
Copy Markdown
Member Author

We can tweak the snapshot generation in a separate PR. But It's probably a small change so if you're interested in tweaking it, I wouldn't mind if it's done in this PR.

How it should be tweaked specifically?

@dummdidumm

Copy link
Copy Markdown
Member

TBH I'm not sure there's a good way to tweak it, because the test generation can't know if the code deviates sufficiently enough from expectedv2 to warrant a new file or not. I would just leave it as is and copy the new v5 contents into the v2 file and then delete the v5 file.

@paoloricciuti

Copy link
Copy Markdown
Member Author

TBH I'm not sure there's a good way to tweak it, because the test generation can't know if the code deviates sufficiently enough from expectedv2 to warrant a new file or not. I would just leave it as is and copy the new v5 contents into the v2 file and then delete the v5 file.

Fixed it

@dummdidumm dummdidumm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you!

@dummdidumm dummdidumm merged commit 80622df into sveltejs:master May 13, 2024
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.

Svelte 5: Whitespace problem in typed parameters to snippets

3 participants