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

CLI: Fix detection of type: module when initializing storybook #18714

Merged
merged 1 commit into from Jul 15, 2022

Conversation

IanVS
Copy link
Member

@IanVS IanVS commented Jul 14, 2022

Issue: storybookjs/builder-vite#441

What I did

I found that the commonJs option being set by initiate through auto-detection of the package.json type: module since #16184 was not actually being used, therefore ESM projects were not being created with main.cjs, causing the error linked above.

Vite recently changed their bootstrap to create projects with "type": "module" in their package.json, which brought this to light.

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

The vite-based end-to-end tests should start to pass again.

@nx-cloud
Copy link

nx-cloud bot commented Jul 14, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 653a9ff. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


Successfully ran 1 target

Sent with 💌 from NxCloud.

yannbf
yannbf approved these changes Jul 14, 2022
Copy link
Member

@yannbf yannbf left a comment

Awesome @IanVS! Thank you so much for this fix.

I wonder if the usage of the commonJs flag should cease to exist in Storybook 7.0.0 though. Once we merge future/base into next, which is planned this week, then I believe this would not be a problem anymore 🤔

@IanVS
Copy link
Member Author

IanVS commented Jul 14, 2022

Sure that makes sense, but I think 7.0 is still a ways off and it would be great to get things working for new vite storybook projects in the meantime I think.

Another thing to consider is using .mjs in 7.0 if the config files are written in esm. I don't think esbuild-register requires it, but it would be more technically correct, I think. Or, we could keep detecting the type from the package.json, and set the extension correctly based on that.

@IanVS IanVS added the patch Bugfix & documentation PR that need to be picked to master branch label Jul 14, 2022
@ahnpnl
Copy link

ahnpnl commented Jul 15, 2022

Can we also add an option to CLI to generate storybook with TypeScript config: main.ts, preview.ts? (separate PR 😃)

@IanVS
Copy link
Member Author

IanVS commented Jul 15, 2022

@ahnpnl there are plans to do just that, for the next version of SB. :)

@IanVS IanVS changed the title Fix detection of type: module when initializing storybook CLI: Fix detection of type: module when initializing storybook Jul 15, 2022
@IanVS IanVS merged commit d1673e1 into next Jul 15, 2022
56 of 59 checks passed
@IanVS IanVS deleted the fix-commonjs-detection branch Jul 15, 2022
@shilman shilman added the picked Patch/release PRs cherry-picked to master/release branch label Jul 26, 2022
shilman pushed a commit that referenced this issue Jul 26, 2022
CLI: Fix detection of type: module when initializing storybook
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cli patch Bugfix & documentation PR that need to be picked to master branch picked Patch/release PRs cherry-picked to master/release branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants