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: correctly migrate sequence expressions #13291

Merged
merged 2 commits into from
Sep 17, 2024

Conversation

paoloricciuti
Copy link
Member

Svelte 5 rewrite

Closes #13278

While working on this i noticed that the sequence expression was a problem for $state too. Also while the migration correctly migrate this

$: double = (console.log(double), count * 2);

the resulting code will actually error at runtime because a derived is accessing himself. This however is a bit harder to migrate effectively because either we should move it to state+effect (kinda 🤮) or we should have an external non stateful variable. Regardless i think we should do this in a separate PR.

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 Sep 17, 2024

🦋 Changeset detected

Latest commit: 213247d

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

@@ -0,0 +1,14 @@
<script>
let count = ($state((void 0, 0)));
Copy link
Member

Choose a reason for hiding this comment

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

this output doesn't seem right

Suggested change
let count = ($state((void 0, 0)));
let count = $state((void 0, 0));

Copy link
Member Author

Choose a reason for hiding this comment

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

The migration simply append $state( and ) to the left and right of the init (it was already like this before).

This keeps the code very simple but if you have multiple parentheses spit out some weird code. Eg.

let count = (((0)));

converts to

let count = ((($state(0))));

Because parentheses are basically counted nowhere in the ast.

We can probably fix this with some more complex code to migrate state but I'm not sure is worth it just for the esthetic.

Copy link
Member Author

Choose a reason for hiding this comment

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

So basically the point is: the last tells us where the expression is but the expression does not include the parentheses and we can't really be sure of how many parentheses, whitespace, code that the ast doesn't consider are there so the code will be probably much more complex (and even worse for handling the derived)

Copy link
Member

Choose a reason for hiding this comment

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

i fixed it 😉

Copy link
Member Author

Choose a reason for hiding this comment

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

Ugh I need to remember that doing such things is fine...whenever I'm working with some AST i tend to avoid touching strings as much as possible but it's actually totally fine. Thanks and sorry 😉

@Rich-Harris Rich-Harris merged commit e3c56fa into main Sep 17, 2024
9 checks passed
@Rich-Harris Rich-Harris deleted the fix-migrate-sequence-expressions branch September 17, 2024 22:08
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.

migrate: incorrect parenthesis handling in derived expressions
2 participants