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

fix: allow multiple optional parameters with defaults #12070

Merged

Conversation

paoloricciuti
Copy link
Member

Svelte 5 rewrite

Closes #12067 i was not totally confident this was an ok fix but i've seen that a similar trick is used in read_type_annotation too so i guess it can be good? Feel free to close if it's not the right fix.

Please note that the Svelte codebase is currently being rewritten for Svelte 5. Changes should target Svelte 5, which lives on the default branch (main).

If your PR concerns Svelte 4 (including updates to svelte.dev.docs), please ensure the base branch is svelte-4 and not main.

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
  • Prefix your PR title with feat:, fix:, chore:, or docs:.
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests and linting

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

Copy link

changeset-bot bot commented Jun 17, 2024

🦋 Changeset detected

Latest commit: f952708

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

This PR includes changesets to release 1 package
Name Type
svelte 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

@Rich-Harris
Copy link
Member

Gah, I thought I had it, but I don't — it fails if the sequence expression isn't the last default:

{#snippet one(a, b = 1, c = (2, 3))}
  {a}{b}{c}
{/snippet}

{#snippet two(a, b = (1, 2), c = 3)}
  {a}{b}{c}
{/snippet}

{@render one(0)}/{@render two(0)}

@paoloricciuti
Copy link
Member Author

Gah, I thought I had it, but I don't — it fails if the sequence expression isn't the last default:

{#snippet one(a, b = 1, c = (2, 3))}
  {a}{b}{c}
{/snippet}

{#snippet two(a, b = (1, 2), c = 3)}
  {a}{b}{c}
{/snippet}

{@render one(0)}/{@render two(0)}

Someone suggested on discord if we could make parse the whole snippet as a function to acorn and then change stuff around.

Do you think it's feasible?

@Rich-Harris
Copy link
Member

Seems worth a try, yeah

@paoloricciuti
Copy link
Member Author

Seems worth a try, yeah

I'll try it out and see what i can come up with.

@paoloricciuti paoloricciuti force-pushed the fix-snippet-optional-parameters branch from fa455c4 to 4b740ae Compare June 21, 2024 16:02
@Rich-Harris
Copy link
Member

@paoloricciuti did you mean to force push? looks like previous commits got obliterated

@paoloricciuti
Copy link
Member Author

@paoloricciuti did you mean to force push? looks like previous commits got obliterated

I just rebased main the hash has changed but the content should be the same.no?

@Rich-Harris
Copy link
Member

My bad — I thought I'd pushed the failing test in #12070 (comment) but I didn't

@paoloricciuti
Copy link
Member Author

@Rich-Harris i still need to work on a bunch of things but this seems to work...just pushing to get some feedback (maybe there are better ways to do what i'm doing or pre-existing functions i'm not aware of).

Comment on lines +277 to +280
while (!parser.match(')') || parentheses !== 1) {
if (parser.match('(')) parentheses++;
if (parser.match(')')) parentheses--;
params += parser.read(/^./);
Copy link
Member

Choose a reason for hiding this comment

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

not a new bug, but this logic probably won't be sufficient because these characters could appear inside strings etc. we should probably fix it both here and inside read_pattern...

Copy link
Member Author

Choose a reason for hiding this comment

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

Uh right...would prevent the plus and minus between strings be sufficient? It should right?

Copy link
Member

Choose a reason for hiding this comment

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

strings, comments and regular expressions

@Rich-Harris
Copy link
Member

just pushed a couple of tweaks — the test is still failing because a node.end is being replaced where it shouldn't — likely something to do with the amend function that normalizes the output of acorn-typescript

@paoloricciuti
Copy link
Member Author

just pushed a couple of tweaks — the test is still failing because a node.end is being replaced where it shouldn't — likely something to do with the amend function that normalizes the output of acorn-typescript

Uhh I like those changes. Very smart. Btw I think this will also fix #12068

@Rich-Harris
Copy link
Member

Opened #12131 for the bracket matching thing, no sense in letting it hold up this fix. Thanks!

@Rich-Harris Rich-Harris merged commit 965c12f into sveltejs:main Jun 21, 2024
7 of 9 checks passed
dummdidumm pushed a commit to sveltejs/language-tools that referenced this pull request Jun 24, 2024
After sveltejs/svelte#12070 the AST for the SnippetBlock has changed (to be more correct).

The problem is that svelte2tsx was using that wrong information for the transformation. This means that currently last version of svelte produces wrong ts code from svelte2tsx.
FoHoOV pushed a commit to FoHoOV/svelte that referenced this pull request Jun 27, 2024
* fix: allow multiple optional parameters with defaults

* Apply suggestions from code review

* partial fix

* feat: parse as a whole function

* couple of fixes

* work around acorn-typescript quirks

* add the harder test

* Update .changeset/ten-geese-share.md

---------

Co-authored-by: Rich Harris <[email protected]>
Co-authored-by: Rich Harris <[email protected]>
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: snippets with multiple default values get incorrectly parsed
2 participants