Skip to content
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

Codemod: Add option to have relative path in csf factories codemod #30463

Open
wants to merge 4 commits into
base: kasper/csf-factories
Choose a base branch
from

Conversation

yannbf
Copy link
Member

@yannbf yannbf commented Feb 4, 2025

Closes #

What I did

This PR introduces a new prompt for the user related to the csf factories codemod.

When using the subpath imports, things may not work if users if they:

  • do not have moduleResolution: bundler in tsconfig
  • use ts path aliases (baseUrl and/or paths property in tsconfig) and a webpack or vite ts paths plugin

So the codemod provides an option for the user to not use subpath imports, and just get relative imports instead in the story files

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

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

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 78 MB 78 MB 0 B 1.38 0%
initSize 132 MB 132 MB 0 B 1.23 0%
diffSize 54 MB 54 MB 0 B 1.14 0%
buildSize 7.2 MB 7.2 MB 0 B 2.2 0%
buildSbAddonsSize 1.85 MB 1.85 MB 0 B 1.15 0%
buildSbCommonSize 195 kB 195 kB 0 B - 0%
buildSbManagerSize 1.86 MB 1.86 MB 0 B -1.19 0%
buildSbPreviewSize 0 B 0 B 0 B - -
buildStaticSize 0 B 0 B 0 B - -
buildPrebuildSize 3.91 MB 3.91 MB 0 B -0.61 0%
buildPreviewSize 3.29 MB 3.29 MB 0 B 1.55 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 10.6s 7.4s -3s -134ms -1.47 🔰-41.9%
generateTime 22.8s 19.3s -3s -491ms -0.68 -18.1%
initTime 13.4s 12.3s -1s -107ms -0.85 -9%
buildTime 9.6s 9.1s -506ms -0.34 -5.5%
testBuildTime 0ms 0ms 0ms - -
devPreviewResponsive 13.7s 5.4s -8s -317ms -0.13 -153.7%
devManagerResponsive 9.4s 3.9s -5s -528ms -0.12 -140.4%
devManagerHeaderVisible 1.8s 790ms -1s -54ms 0.12 -133.4%
devManagerIndexVisible 1.8s 800ms -1s -60ms 0.04 -132.5%
devStoryVisibleUncached 9.4s 4.1s -5s -353ms 0.58 -130.5%
devStoryVisible 1.9s 821ms -1s -135ms 0.08 -138.2%
devAutodocsVisible 1.9s 757ms -1s -181ms 0.23 -156%
devMDXVisible 1.7s 790ms -959ms 0.46 -121.4%
buildManagerHeaderVisible 2s 774ms -1s -257ms 0.19 -162.4%
buildManagerIndexVisible 2.1s 776ms -1s -329ms 0.16 -171.3%
buildStoryVisible 2s 761ms -1s -244ms 0.21 -163.5%
buildAutodocsVisible 1.5s 746ms -821ms 0.71 -110.1%
buildMDXVisible 1.7s 628ms -1s -77ms 0.17 -171.5%

Greptile Summary

Based on the provided information, I'll create a concise summary of the key changes in this PR:

Added user prompt and relative path import option to the CSF factories codemod, providing flexibility in how preview configs are imported in stories.

  • Added new useSubPathImports option in storyToCsfFactory to support both subpath (#.storybook/preview) and relative path imports
  • Modified path calculation logic in story-to-csf-factory.ts to handle relative paths from story files to preview config
  • Added conditional imports map configuration in package.json only when subpath imports are chosen
  • Added idempotency check in config-to-csf-factory.ts to prevent re-transformation of already converted code
  • Improved import detection and handling to properly manage type imports and prevent duplicates

Copy link

nx-cloud bot commented Feb 4, 2025

View your CI Pipeline Execution ↗ for commit 359ec9c.

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

☁️ Nx Cloud last updated this comment at 2025-02-04 15:39:02 UTC

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.

4 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings | Greptile

Comment on lines 133 to +136
t.isImportDeclaration(node) &&
node.source.value === configImport.source.value &&
!node.importKind
!t.isImportSpecifier(node) &&
node.importKind !== 'type' &&
node.source.value === configImport.source.value
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: The check !t.isImportSpecifier(node) is redundant since isImportDeclaration(node) already implies this

Suggested change
t.isImportDeclaration(node) &&
node.source.value === configImport.source.value &&
!node.importKind
!t.isImportSpecifier(node) &&
node.importKind !== 'type' &&
node.source.value === configImport.source.value
t.isImportDeclaration(node) &&
node.importKind !== 'type' &&
node.source.value === configImport.source.value

Comment on lines +59 to +63
const relativePath = relative(dirname(info.path), previewConfigPath)
// Convert Windows backslashes to forward slashes if present
.replace(/\\/g, '/')
// Remove .ts or .js extension
.replace(/\.(ts|js)x?$/, '');
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: Path calculation could fail if previewConfigPath is not absolute. Consider using path.resolve() to ensure absolute paths.

dryRun,
packageManager,
useSubPathImports,
previewConfigPath: previewConfigPath!,
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: non-null assertion on previewConfigPath could cause runtime errors if undefined

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant