Skip to content

Svelte: Fix conflicting variable names and support for +page.svelte files#30369

Merged
JReinhold merged 4 commits intostorybookjs:nextfrom
xeho91:next
Feb 11, 2025
Merged

Svelte: Fix conflicting variable names and support for +page.svelte files#30369
JReinhold merged 4 commits intostorybookjs:nextfrom
xeho91:next

Conversation

@xeho91
Copy link
Contributor

@xeho91 xeho91 commented Jan 24, 2025

Closes #29636
Closes #30212

What I did

I've replaced the previous implementation of getting the component name - which was copied from Svelte internals. It doesn't cover edge cases.

Now, it access the AST of compiled output of Svelte file. I believe this is a final source of truth to avoid naming collision with other identifiers. This function handles legacy and modern version of Svelte.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

+page.svelte components
  1. Create a svelte-vite sandbox
  2. In the sandbox src create a +page.svelte file and fill it with whatever text you want
  3. create a +page.stories.ts file with:
import Page from './+page.svelte';

export default {
  component: Page,
  tags: ['autodocs']
}

export const Default = {};
  1. Open the newly created story and its Docs page, and ensure both render without issues.
Conflicting variables
  1. In the existing svelte-vite sandbox
  2. Create a Greeting.svelte file with (the trick here is that there is a variable name with the same as the file name):
<script>
	let Greeting = 'world';
</script>

Hello {Greeting}
  1. Create a Greeting.stories.ts file with:
import Greeting from './Greeting.svelte';

export default {
  component: Greeting,
  tags: ['autodocs']
}

export const Default = {};
  1. Ensure that both the story and its Docs page renders without issues.

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli-storybook/src/sandbox-templates.ts

  • Make sure this PR contains one of the labels below:

    Available labels
    • bug: Internal changes that fixes incorrect behavior.
    • maintenance: User-facing maintenance tasks.
    • dependencies: Upgrading (sometimes downgrading) dependencies.
    • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
    • cleanup: Minor cleanup style change. Will not show up in release changelog.
    • documentation: Documentation only changes. Will not show up in release changelog.
    • feature request: Introducing a new feature.
    • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
    • other: Changes that don't fit in the above categories.

🦋 Canary release

This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the @storybookjs/core team here.

core team members can create a canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>

name before after diff z %
createSize 0 B 0 B 0 B - -
generateSize 80.5 MB 80.5 MB 315 B 0.85 0%
initSize 80.5 MB 80.5 MB 315 B -0.47 0%
diffSize 97 B 97 B 0 B -0.5 0%
buildSize 7.24 MB 7.24 MB 0 B 0.62 0%
buildSbAddonsSize 1.87 MB 1.87 MB 0 B 0.2 0%
buildSbCommonSize 195 kB 195 kB 0 B - 0%
buildSbManagerSize 1.88 MB 1.88 MB 0 B -0.05 0%
buildSbPreviewSize 0 B 0 B 0 B - -
buildStaticSize 0 B 0 B 0 B - -
buildPrebuildSize 3.94 MB 3.94 MB 0 B 0.11 0%
buildPreviewSize 3.3 MB 3.3 MB 0 B 0.81 0%
testBuildSize 0 B 0 B 0 B - -
testBuildSbAddonsSize 0 B 0 B 0 B - -
testBuildSbCommonSize 0 B 0 B 0 B - -
testBuildSbManagerSize 0 B 0 B 0 B - -
testBuildSbPreviewSize 0 B 0 B 0 B - -
testBuildStaticSize 0 B 0 B 0 B - -
testBuildPrebuildSize 0 B 0 B 0 B - -
testBuildPreviewSize 0 B 0 B 0 B - -
name before after diff z %
createTime 26s 24.8s -1s -116ms 0.49 -4.5%
generateTime 21s 18.5s -2s -498ms -0.68 -13.5%
initTime 4.4s 4.1s -309ms -0.65 -7.5%
buildTime 9.6s 8.1s -1s -506ms -1.1 -18.4%
testBuildTime 0ms 0ms 0ms - -
devPreviewResponsive 6.1s 5.3s -764ms -0.04 -14.2%
devManagerResponsive 4.8s 3.8s -940ms -0.34 -24.3%
devManagerHeaderVisible 834ms 692ms -142ms -0.68 -20.5%
devManagerIndexVisible 849ms 723ms -126ms -0.79 -17.4%
devStoryVisibleUncached 2.3s 2.8s 500ms -1.5 🔺17.8%
devStoryVisible 1s 748ms -312ms -0.65 -41.7%
devAutodocsVisible 710ms 687ms -23ms -0.39 -3.3%
devMDXVisible 742ms 696ms -46ms -0.36 -6.6%
buildManagerHeaderVisible 750ms 741ms -9ms -0.36 -1.2%
buildManagerIndexVisible 840ms 838ms -2ms -0.39 -0.2%
buildStoryVisible 677ms 674ms -3ms -0.38 -0.4%
buildAutodocsVisible 549ms 572ms 23ms -0.28 4%
buildMDXVisible 606ms 600ms -6ms -0.37 -1%

Greptile Summary

This PR updates the component name extraction logic in the Svelte docgen plugin to handle both Svelte 4 and 5 compatibility by using compiled AST output.

  • Added getComponentName function in code/frameworks/svelte-vite/src/plugins/svelte-docgen.ts to extract names from compiled AST
  • Added IS_SVELTE_V4 flag in code/frameworks/svelte-vite/src/utils.ts to handle version-specific logic
  • Fixed component name extraction for files with special characters (e.g. +page.svelte)
  • Fixed reference errors when variables match component names in Svelte 5

💡 (4/5) You can add custom instructions or style guidelines for the bot here!

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

2 file(s) reviewed, 5 comment(s)
Edit PR Review Bot Settings | Greptile

@xeho91
Copy link
Contributor Author

xeho91 commented Jan 24, 2025

@JReinhold

I wanted to write tests for this case, but given that the changes were applied to svelte-vite package directory, it doesn't have any tests. So, I am puzzled as to where to add.

And also a quick question. Should I make three separate errors inside getComponentName() or should it be one error extending the one from Storybook?

@nx-cloud
Copy link

nx-cloud bot commented Jan 24, 2025

View your CI Pipeline Execution ↗ for commit e0154a5.

Command Status Duration Result
nx run-many -t build --parallel=3 ✅ Succeeded 1m 58s View ↗

☁️ Nx Cloud last updated this comment at 2025-02-11 19:27:16 UTC

@storybook-app-bot
Copy link

storybook-app-bot bot commented Feb 3, 2025

Package Benchmarks

Commit: e0154a5, ran on 11 February 2025 at 19:27:00 UTC

No significant changes detected, all good. 👏

@JReinhold JReinhold changed the title fix(svelte-vite): Use compiled AST output as source of truth for getting component name Svelte: Fix conflicting variable names and support for +page.svelte files Feb 11, 2025
@JReinhold JReinhold merged commit e0154a5 into storybookjs:next Feb 11, 2025
52 of 54 checks passed
@JReinhold
Copy link
Contributor

Oh, I completely screwed up git while trying to push a fix to this branch, accidentally merging it. 🙃 If it works I'll just leave as is, otherwise I'll revert it.

I wanted to write tests for this case, but given that the changes were applied to svelte-vite package directory, it doesn't have any tests. So, I am puzzled as to where to add.

You can just create a Vitest test file in code/frameworks/svelte-vite/src/plugins/svelte-docgen.test.ts and run yarn test:watch svelte-docgen in the code dir to run the test, Vitest is fully set up even though there are no tests yet. 🙂

And also a quick question. Should I make three separate errors inside getComponentName() or should it be one error extending the one from Storybook?

I think the raw errors you have here are fine, given that this should be unreachable. Users shouldn't reach these cases even if they do something wrong (right?), so it's more a sanity check than an actual guide for users.

@JReinhold
Copy link
Contributor

CI is passing and I've manually checked that it fixes the two issues. Great work. 💪

https://app.circleci.com/pipelines/github/storybookjs/storybook/91681/workflows/2ad4eb13-a4fe-48de-929e-d671d640a455

@JReinhold JReinhold added needs qa Indicates that this needs manual QA during the upcoming minor/major release and removed needs qa Indicates that this needs manual QA during the upcoming minor/major release labels Feb 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

3 participants