Vite: Use vite hook filter for performance improvements#34022
Vite: Use vite hook filter for performance improvements#34022huang-julien wants to merge 5 commits intonextfrom
Conversation
📝 WalkthroughWalkthroughRefactors multiple Vite plugin hooks from function-based signatures to structured objects exposing filter and handler (or resolver filter/handler), preserving existing logic and observable outputs across builders and framework plugins. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Sidnioulz
added
performance issue
builder-vite
ci:daily
Sidnioulz
changed the title
|
View your CI Pipeline Execution ↗ for commit 381d7c4
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/frameworks/vue3-vite/src/plugins/vue-component-meta.ts`:
- Around line 126-132: The guard that tries to detect re-exports incorrectly
matches local exports because it uses unescaped name in loose regexes; update
the check in vue-component-meta.ts that uses the variables name and src so it
only treats true re-exports as re-exports: first escape name with a proper
escapeRegExp helper before composing regexes, and change the first pattern to
require a from clause (e.g. only match "export { ... } from ...") instead of
matching any "export { ... }" so local "export { Foo }" is not treated as a
re-export; also avoid using \b around name (or only apply \b when name matches
JS identifier characters) so names with non-word chars like "$Foo" are handled
correctly. Ensure the modified checks preserve the other re-export branch
(export \* from ...) and still fall back to !src.includes(name) where
appropriate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 53a0d31a-a051-4e3c-8e5e-5b020a5c4f0c
📒 Files selected for processing (9)
code/addons/docs/src/mdx-plugin.tscode/builders/builder-vite/src/plugins/inject-export-order-plugin.tscode/builders/builder-vite/src/plugins/storybook-external-globals-plugin.tscode/builders/builder-vite/src/plugins/strip-story-hmr-boundaries.tscode/builders/builder-vite/src/plugins/vite-inject-mocker/plugin.tscode/builders/builder-vite/src/plugins/vite-mock/plugin.tscode/frameworks/svelte-vite/src/plugins/svelte-docgen.tscode/frameworks/vue3-vite/src/plugins/vue-component-meta.tscode/frameworks/vue3-vite/src/plugins/vue-docgen.ts
| if ( | ||
| new RegExp(`export {.*${name}.*}`).test(src) || | ||
| new RegExp(`export \\* from ['"]\\S*${name}['"]`).test(src) || | ||
| // when using re-exports, some exports might be resolved via checker.getExportNames | ||
| // but are not directly exported inside the current file so we need to ignore them too | ||
| !src.includes(name) | ||
| ) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "vue-component-meta.ts"Repository: storybookjs/storybook
Length of output: 124
🏁 Script executed:
fd -type f -path "*vue-component-meta*"Repository: storybookjs/storybook
Length of output: 235
🏁 Script executed:
git ls-files | grep "vue-component-meta"Repository: storybookjs/storybook
Length of output: 400
🏁 Script executed:
# Test the proposed fix regex as well
node - <<'JS'
function proposedGuard(name, src) {
const escapedName = name.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
const isNamedReExport = new RegExp(
`export\\s*\\{[^}]*\\b${escapedName}\\b[^}]*\\}\\s*from\\s*['"][^'"]+['"]`
).test(src);
return isNamedReExport || !src.includes(name);
}
const cases = [
{ name: 'Foo', src: 'const Foo = {}; export { Foo };', expectedSkip: false, why: 'local named export should be injectable' },
{ name: 'Foo', src: "export { default as Foo } from './Foo.vue';", expectedSkip: true, why: 'named re-export should be skipped' },
{ name: '$Foo', src: "export { default as $Foo } from './Foo.vue';", expectedSkip: true, why: 'regex should still treat name literally' },
];
console.log("\nProposed guard behavior:");
console.log("Name | Actual | Expected | Reason");
console.log("-----|--------|----------|-------");
for (const c of cases) {
const actual = proposedGuard(c.name, c.src);
const match = actual === c.expectedSkip ? "✓" : "✗";
console.log(`${match} ${c.name} | ${actual} | ${c.expectedSkip} | ${c.why}`);
}
JSRepository: storybookjs/storybook
Length of output: 342
🏁 Script executed:
# Get the file size and structure first
wc -l code/frameworks/vue3-vite/src/plugins/vue-component-meta.tsRepository: storybookjs/storybook
Length of output: 128
🏁 Script executed:
# Find the extractArgTypes file
fd -t f "extractArgTypes.ts" | grep vue3Repository: storybookjs/storybook
Length of output: 107
🏁 Script executed:
# Check if files actually exist in repo
git ls-files | grep "vue3" | head -20Repository: storybookjs/storybook
Length of output: 1024
🏁 Script executed:
# Try to locate and read the actual file with proper paths
git ls-files | grep "vue3-vite" | grep "vue-component-meta"Repository: storybookjs/storybook
Length of output: 124
🏁 Script executed:
# List structure of code/frameworks directory
ls -la code/frameworks/ 2>/dev/null | head -20 || echo "Directory might not exist, checking git structure"
git ls-files "code/frameworks/vue3-vite/*" | head -10Repository: storybookjs/storybook
Length of output: 1473
🏁 Script executed:
# Get the actual content from lines around 126-132
git show HEAD:code/frameworks/vue3-vite/src/plugins/vue-component-meta.ts 2>/dev/null | head -140 | tail -25Repository: storybookjs/storybook
Length of output: 1366
🏁 Script executed:
# Also check the downstream file
git show HEAD:code/renderers/vue3/src/extractArgTypes.ts 2>/dev/null | head -30Repository: storybookjs/storybook
Length of output: 1200
🏁 Script executed:
# Verify the word boundary issue with $Foo more explicitly
node - <<'JS'
const escapedName = "$Foo".replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
const pattern = `export\\s*\\{[^}]*\\b${escapedName}\\b[^}]*\\}\\s*from`;
const regex = new RegExp(pattern);
const src = "export { default as $Foo } from './Foo.vue';";
console.log(`Escaped name: ${escapedName}`);
console.log(`Pattern: ${pattern}`);
console.log(`Testing: "${src}"`);
console.log(`Matches: ${regex.test(src)}`);
// Test without word boundary
const patternNoWB = `export\\s*\\{[^}]*${escapedName}[^}]*\\}\\s*from`;
const regexNoWB = new RegExp(patternNoWB);
console.log(`\nWithout word boundaries:`);
console.log(`Pattern: ${patternNoWB}`);
console.log(`Matches: ${regexNoWB.test(src)}`);
JSRepository: storybookjs/storybook
Length of output: 289
Current regex guard incorrectly skips valid local exports due to unescaped name parameter and overly broad pattern.
The first regex export {.*${name}.*} matches both local exports like export { Foo } and re-exports, failing to distinguish between them and causing valid local components to be skipped without __docgenInfo injection. The name parameter is not regex-escaped, so special characters (e.g., $, [) are misinterpreted as regex metacharacters instead of literals. This suppresses argTypes downstream, as extractArgTypes.ts returns null when __docgenInfo is missing.
The proposed fix improves pattern specificity by requiring the from clause, but its word boundary anchators fail when name contains special characters like $Foo (word boundaries don't work correctly when adjacent to non-word characters like $ or spaces). Consider removing word boundaries for names that may contain special characters, or validating against the character set of valid Vue export identifiers.
🧰 Tools
🪛 ast-grep (0.41.0)
[warning] 126-126: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(export {.*${name}.*})
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 127-127: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(export \\* from ['"]\\S*${name}['"])
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/frameworks/vue3-vite/src/plugins/vue-component-meta.ts` around lines 126
- 132, The guard that tries to detect re-exports incorrectly matches local
exports because it uses unescaped name in loose regexes; update the check in
vue-component-meta.ts that uses the variables name and src so it only treats
true re-exports as re-exports: first escape name with a proper escapeRegExp
helper before composing regexes, and change the first pattern to require a from
clause (e.g. only match "export { ... } from ...") instead of matching any
"export { ... }" so local "export { Foo }" is not treated as a re-export; also
avoid using \b around name (or only apply \b when name matches JS identifier
characters) so names with non-word chars like "$Foo" are handled correctly.
Ensure the modified checks preserve the other re-export branch (export \* from
...) and still fall back to !src.includes(name) where appropriate.
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
2 participants
reopening closed #33900 as changes was started on the wrong branch.
Closes #33899
What I did
This PR moves Vite plugins to use
HookObjects instead of functions.For example
transform(code, id)would move toThis allows to reduce the overhead between JS/Rust runtimes for vite 8 with rolldown. Implementation within handlers doesn't change,
filter()are left within to keep plugins backward compatible with vite < 6.3Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
We need to test all affected vite plugin. Just to ensure they run correctly
Caution
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-storybook/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 publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit