Vite: Improve handling of preview annotations #28798
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM
5 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 502248b. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
|
Thank you for the PR @tobiasdiez. Could you have a look at why the CI is failing? |
Tests are passing now. |
ndelangen
changed the title
|
@tobiasdiez I think I may have broken something when i merged in the base branch, but I'm not sure what. Can you assist? |
Thanks for your help! The tests should pass now, but there are ts errors during build. I have no idea where they are coming from. Do you? |
| if (isAbsolute(path)) { | ||
| return normalize(path); | ||
| } |
There was a problem hiding this comment.
Doesn't this pose a security vulnerability? Leaking the build-environment's filesystem directory-structure to the client bundle? I'm not a security expert at all, I just recall a few CVEs coming our way for exactly this, and I know we go out of our way in other places to ensure that nothing above the project root ends up in the bundle.
And maybe I'm misunderstanding this whole thing, and this doesn't actually end up in the bundle, and then it's fine. I was mostly just gauging this from the new tests.
There was a problem hiding this comment.
No, this only "leaks" the full path to vite - but vite will then replace it with the import of the correct output chunk upon bundling.
|
Managed to fix the tests, so this is good to go from my side. @ndelangen @tmeasday please review when you have a bit of spare time. Thanks! |
There was a problem hiding this comment.
I think we need someone who is more familiar with the vite builder to review this.
I have a couple qns:
Are we absolutely (haha) sure the absolute paths don't end up in the bundle anywhere? I'd like to verify this.
This is fixed by using normalize from the pathe package, which converts this to /."
Should we use slash for this which is an existing dependency?
| projectRoot, | ||
| frameworkName: 'frameworkName', | ||
| }); | ||
| expect(result).toMatchSnapshot(); |
There was a problem hiding this comment.
Usually we prefer toMatchInlineSnapshot()
|
What I did
While working on the Nuxt integration, I noticed a couple of issues with how the preview annotations are handled. Namely:
optimizeDeps(one of them is from Vite: Add jsdoc-type-pratt-parser tooptimizeDeps#29179).node_moduleslikenode_modules/packageA/node_modules/storybookare truncated to the last package (storybookin the example), which then cannot be resolved. This is fixed by always resolving the preview annotation paths to absolute paths and using them. The code contained a few remarks that vite appearently has problems with absolute paths, but as far as I'm aware vite handles absolute paths perfectly fine.\as path separator. This is fixed by usingnormalizefrom thepathepackage, which converts this to/.The code to create the preview annotation imports is inspired by https://github.com/nuxt/nuxt/blob/754fc30e5d0fe506ee3218f9c4a11fa047e3553f/packages/nuxt/src/core/templates.ts#L61-L76
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
I've run and added a few tests. Still have to figure out how to test the sandboxes.
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli/src/sandbox-templates.tsMake 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/coreteam 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>Greptile Summary
Improved handling of preview annotations in the Vite builder for Storybook, addressing path normalization and import issues.
code/builders/builder-vite/package.json: Addedknitwork,pathe, andslashdependencies for better path handling.code/builders/builder-vite/src/codegen-modern-iframe-script.ts: Updated to generate absolute paths for preview annotations and improved HMR logic.code/builders/builder-vite/src/utils/process-preview-annotation.ts: Replacedpathwithpathefor normalization and added deprecation warning for object-based annotations.code/builders/builder-vite/src/codegen-modern-iframe-script.test.ts: Added unit tests forgenerateModernIframeScriptCodeFromPreviews.code/builders/builder-vite/src/utils/process-preview-annotation.test.ts: Updated tests to ensure correct path handling, especially for Windows paths.