Skip to content

fix: correct start of {:else if} and {:else} #12043

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

Merged
merged 6 commits into from
Jul 8, 2024
Merged

Conversation

baseballyama
Copy link
Member

@baseballyama baseballyama commented Jun 16, 2024

Svelte 5 rewrite

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

Currently, the AST node for {:else if} and {:else} has its start pointing to the start of the body. However, it should point to the start of the block.

image

https://svelte-5-preview.vercel.app/#H4sIAAAAAAAAE2WQu27DMAxFf4VQl7h1amd1HkDHDl2KbnUHQaYTAhJlSFSQwvC_FzKcPtJBD-rcyytpVD1ZjKp5HxVrh6pRT8OgSiWfQy7iGa2gKlX0KZh8sosm0CCHllvpExshz3BEedXceffMsnLEJTh9KWDMIgBHDHt40XJ6NEg2C4rtgvTlinrrfVhl38ICSgr8G87bMAetCriHrIZ17l_Aw7xsoareTpj7kksOKAJejE2RzgiaO5AMia-QeIE5cspTHsZzFDA-scD-7-PWm7qETT3fkXfVz2fweEf94jlAPfca53LKQceAWjCAnDRD3fLYoI0I35bdf4vFGG_0t5IMKuqnllWpnO-oJ-xUIyHh9DF9AVb2PN_dAQAA

With the PR, AST will be like this.

image

Copy link

changeset-bot bot commented Jun 16, 2024

🦋 Changeset detected

Latest commit: a46c2de

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

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

Why the change? What's currently there mimics what you get from the Svelte 4 AST - if you paste the code into the Svelte 4 REPL you get the same AST as on Svelte 5 preview.

If there is a good reason to change this we need to make sure to adjust the position to the start of the body when converting to the legacy AST.

@baseballyama
Copy link
Member Author

baseballyama commented Jun 17, 2024

On the AST, even though {#if} and {#else if} both use IfBlock, the start character of {#if} becomes start, while for {#else if}, the consequent part becomes start. This discrepancy should be corrected.

I have checked this in Svelte 4 now, and I think this should fix in Svelte 4 as well, I think. If there is an issue in some Svelte ecosystem, I can implement converting logic for the legacy AST.

@baseballyama
Copy link
Member Author

baseballyama commented Jun 17, 2024

Why the change?

I am currently working on component-level tree shaking and needed this.
Now start of IfBlock is different in {#if} and {#else if}, so it's difficult to know the actual start of the block when I walk AST.

@dummdidumm
Copy link
Member

I'm probably ok with changing the behavior for the new AST, but we absolutely should keep it as-is for the legacy AST. I'm pretty sure there's logic in language tools / prettier that relies on the start position being that way.

@Rich-Harris
Copy link
Member

I kinda wonder if we should change the shape of the AST in this case rather than just moving the start, because no start position is exactly correct (though I do think the start in this PR makes more sense than the status quo, aside from any questions about whether it's a worthwhile change).

Something like this:

export interface IfBlock extends BaseNode {
  type: 'IfBlock';
-  elseif: boolean;
  test: Expression;
  consequent: Fragment;
-  alternate: Fragment | null;
+  alternate: ElseIfBlock | Fragment | null;
}

+export interface ElseIfBlock extends IfBlock {
+  type: 'ElseIfBlock'
+}

@baseballyama
Copy link
Member Author

It may be noted that if ElseIfBlock is used, the shape will be slightly different from the JavaScript AST.
(acorn and other parsers are using IfStatement for else if.)

image

In any case, it is strange that the start of the IfBlock is a larger number than the start of the test node, so it is better to fix this in some way.
In the meantime, I will add the conversion code to legacy AST.

@baseballyama
Copy link
Member Author

@dummdidumm @Rich-Harris

In my personal conclusion, I believe it’s better to use IfBlock and just modify the start property, rather than creating a separate ElseIfBlock.

Reason 1: It’s preferable to keep the structure consistent with the JavaScript AST. Different AST structures for JavaScript and Svelte IF statements can cause confusion. It’s better if only the loc property differs slightly.

Reason 2: DRY principle regression.
There are 14 logic locations related to IfBlock, with most of the processing being common between IfBlock and ElseIfBlock. Separating them could increase the risk of implementation errors due to duplicate code.

@baseballyama baseballyama requested a review from dummdidumm June 29, 2024 05:48
@dummdidumm dummdidumm merged commit 67bf7a8 into main Jul 8, 2024
9 checks passed
@dummdidumm dummdidumm deleted the fix/elseif-else-start branch July 8, 2024 09:46
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.

3 participants