React: Improve import rewriting when tsconfig paths are used#33072
Conversation
|
View your CI Pipeline Execution ↗ for commit abdc858
☁️ Nx Cloud last updated this comment at |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds an npm-like package name validator and uses it to guard import-rewrite logic: imports are rewritten to the provided packageName only when the stripped import identifier is not a valid package name, preserving valid scoped/bare specifiers and existing overrideSource behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as getComponentImports
participant Validator as valid-package-name
participant Decision as Rewrite Logic
Caller->>Decision: receive importId, packageName, overrideSource?
Decision->>Validator: stripSubpath(importId) -> top
Decision->>Validator: validPackageName(top)?
alt valid
Decision-->>Caller: return original import (no rewrite)
else
alt overrideSource present
Decision-->>Caller: use overrideSource / rewrite -> packageName
else
Decision-->>Caller: rewrite import -> packageName
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
✨ Finishing touches
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-10-01T15:24:01.060ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (1)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
code/renderers/react/src/componentManifest/getComponentImports.test.ts (1)
588-667: Good test coverage for new rewrite behavior.The three new tests effectively cover:
- Tilde-prefixed sources being rewritten to
packageName- Hash-prefixed sources being rewritten to
packageName- Scoped package subpaths remaining unchanged (not rewritten)
Consider adding a test for simple unscoped bare specifiers with
packageNameprovided:test('Does not rewrite simple unscoped package (valid bare specifier)', () => { const code = dedent` import { useState } from 'react'; const meta = {}; export default meta; export const S = () => { const [x] = useState(); return <div />; }; `; expect(getImports(code, 'pkg')).toMatchInlineSnapshot(` { "components": [], "imports": [], } `); });This would verify that valid unscoped packages like
"react"are not rewritten whenpackageNameis provided, and would catch the bug identified in valid-package-name.ts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
code/renderers/react/src/componentManifest/getComponentImports.test.ts(1 hunks)code/renderers/react/src/componentManifest/getComponentImports.ts(2 hunks)code/renderers/react/src/componentManifest/valid-package-name.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32594
File: code/core/src/components/components/Popover/WithPopover.tsx:7-9
Timestamp: 2025-10-01T15:24:01.060Z
Learning: In the Storybook repository, "react-aria-components/patched-dist/*" (e.g., "react-aria-components/patched-dist/Dialog", "react-aria-components/patched-dist/Popover", "react-aria-components/patched-dist/Tooltip") are valid import paths created by a patch applied to the react-aria-components package. These imports should not be flagged as broken or invalid until a maintainer explicitly states they are no longer acceptable.
📚 Learning: 2025-10-01T15:24:01.060Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32594
File: code/core/src/components/components/Popover/WithPopover.tsx:7-9
Timestamp: 2025-10-01T15:24:01.060Z
Learning: In the Storybook repository, "react-aria-components/patched-dist/*" (e.g., "react-aria-components/patched-dist/Dialog", "react-aria-components/patched-dist/Popover", "react-aria-components/patched-dist/Tooltip") are valid import paths created by a patch applied to the react-aria-components package. These imports should not be flagged as broken or invalid until a maintainer explicitly states they are no longer acceptable.
Applied to files:
code/renderers/react/src/componentManifest/getComponentImports.test.tscode/renderers/react/src/componentManifest/getComponentImports.ts
🧬 Code graph analysis (2)
code/renderers/react/src/componentManifest/getComponentImports.test.ts (1)
code/renderers/react/src/componentManifest/getComponentImports.ts (1)
getImports(244-496)
code/renderers/react/src/componentManifest/getComponentImports.ts (1)
code/renderers/react/src/componentManifest/valid-package-name.ts (2)
validPackageName(19-67)stripSubpath(4-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (3)
code/renderers/react/src/componentManifest/getComponentImports.ts (2)
9-9: LGTM - Import additions support new validation logic.The new imports enable package-name validation for import rewriting.
280-285: Import rewrite logic updated to use package-name validation.The new condition
!validPackageName(stripSubpath(importId))correctly gates rewrites: imports are rewritten topackageNameonly when the stripped import identifier is not a valid package name. This preserves valid bare specifiers (including scoped packages with subpaths) while rewriting relative paths, tilde-prefixed, hash-prefixed, and other invalid specifiers.However, this logic relies on the correctness of
validPackageName- see critical issue flagged in valid-package-name.ts.code/renderers/react/src/componentManifest/valid-package-name.ts (1)
4-17: LGTM - Subpath stripping logic is correct.The
stripSubpathfunction correctly extracts the base package name, handling both scoped (@scope/pkg/subpath→@scope/pkg) and unscoped (react/jsx-runtime→react) forms.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: Jeppe Reinhold <jeppe@chromatic.com>
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
|
@kasperpeulen I've opened a new pull request, #33075, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Pull Request Overview
This PR improves how React imports are rewritten when TypeScript path aliases (like ~/components/Button or #Button) are used. Instead of only rewriting relative imports (those starting with .), the new logic rewrites any import that is not a valid npm package name to the provided package name.
Key Changes
- Added utility functions
stripSubpathandvalidPackageNameto validate npm package names - Changed import rewriting logic from checking
isRelative()to checking!validPackageName(stripSubpath(importId)) - Added comprehensive test coverage for tilde-prefixed, hash-prefixed, and scoped package imports
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
code/renderers/react/src/componentManifest/valid-package-name.ts |
New file with utilities to strip subpaths from package specifiers and validate npm package names |
code/renderers/react/src/componentManifest/getComponentImports.ts |
Updated import rewriting logic to use package name validation instead of relative path check |
code/renderers/react/src/componentManifest/getComponentImports.test.ts |
Added tests for tilde/hash-prefixed imports and scoped packages with subpaths |
Co-authored-by: kasperpeulen <1035299+kasperpeulen@users.noreply.github.com>
Closes #
What I did
This makes sure that imports in the story that are not valid package names:
are rewritten to the package name where the component is located in:
If the package is a valid package
we keep the import.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
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 pull request has been released as version
0.0.0-pr-33072-sha-abdc858b. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-33072-sha-abdc858b sandboxor in an existing project withnpx storybook@0.0.0-pr-33072-sha-abdc858b upgrade.More information
0.0.0-pr-33072-sha-abdc858bkasper/importsabdc858b1763479423)To request a new release of this pull request, mention the
@storybookjs/coreteam.core team members can create a new canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=33072Summary by CodeRabbit
New Features
Refactor
Tests