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

bug: always create main with cjs extension #18648

Merged
merged 2 commits into from Jul 9, 2022

Conversation

benmccann
Copy link
Contributor

@benmccann benmccann commented Jul 6, 2022

Issue: #9854

What I did

Fixed an issue in npx sb init for SvelteKit projects

What extension svelte.config.js uses is irrelevant. Storybook doesn't support ESM, so you have to use the .cjs extension if package.json contains "type": "module" (which all new SvelteKit projects do). Just always using .cjs is easiest and will always work

How to test

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

If your answer is yes to any of these, please make sure to include it in your PR.

@nx-cloud
Copy link

nx-cloud bot commented Jul 6, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 70d2e77. 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.

@ndelangen
Copy link
Member

ndelangen commented Jul 6, 2022

This only affects the svelte generator? Yeah should be OK I guess!

@benmccann
Copy link
Contributor Author

benmccann commented Jul 6, 2022

The Svelte generator is the only one that uses the commonJs option. I updated this file alone because I'm just trying to get Storybook working with SvelteKit (sveltejs/kit#5397)

It wouldn't be a bad idea for Storybook to just remove the option and always create the file with a .cjs extension since that would make Storybook work in any project regardless of whether the user adds "type": "module". Or alternatively Storybook could read the package.json and check the value of the type field to decide on the extension to use, but that would only make it work in the case that the user updates the type field and then runs the generator and not the other way around. But making either of those changes seemed outside the scope of just getting this working for Svelte projects

@ndelangen
Copy link
Member

ndelangen commented Jul 7, 2022

I'll bring it up with the team, thanks for the suggestion!

@ndelangen
Copy link
Member

ndelangen commented Jul 9, 2022

I'm pretty sure with our change in 7.0 (future/base branch) this whole commonjs option/toggle can be removed. Users will be able to write CJS, TS or MJS files, and it will all just work, thanks to esbuild-register

@ndelangen ndelangen merged commit e7ad108 into storybookjs:next Jul 9, 2022
36 of 38 checks passed
@shilman
Copy link
Member

shilman commented Jul 9, 2022

@benmccann @ndelangen Should we patch this back to 6.5?

@benmccann
Copy link
Contributor Author

benmccann commented Jul 9, 2022

Yes, please

@benmccann benmccann deleted the patch-4 branch Jul 9, 2022
@shilman shilman added the patch Bugfix & documentation PR that need to be picked to master branch label Jul 9, 2022
@yannbf
Copy link
Member

yannbf commented Jul 13, 2022

I'd say this change should be released in 6.5 and then undone for 7.0.0, given @ndelangen's explanation with esbuild-register!

@shilman shilman mentioned this pull request Jul 25, 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
bug: always create main with cjs extension
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app: svelte bug 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