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

Default sb init in ESM projects to use cjs #16184

Merged
merged 2 commits into from
Sep 30, 2021
Merged

Conversation

kylegach
Copy link
Contributor

@kylegach kylegach commented Sep 28, 2021

Discovered the following issue (edit: also reported here: #16181):

  1. Start a new project with lit-element-starter-js
  2. Run npx sb init
  3. Run npm run storybook
  4. Receive the following error:
    ERR! Error [ERR_REQUIRE_ESM]: Must use import to load ES Module: <path-to-project>/.storybook/main.js
    ERR! require() of ES modules is not supported.
    ERR! require() of <path-to-project>/.storybook/main.js from <path-to-project>/node_modules/@storybook/core-common/dist/cjs/utils/interpret-require.js is an ES module file as it is a .js file whose nearest parent package.json contains "type": "module" which defines all .js files in that package scope as ES modules.
    ERR! Instead rename main.js to end in .cjs, change the requiring code to use import(), or remove "type": "module" from <path-to-project>/package.json.
    ERR!
    ERR!     at Object.Module._extensions..js (internal/modules/cjs/loader.js:1089:13)
    ERR!     at Module.load (internal/modules/cjs/loader.js:937:32)
    ERR!     at Function.Module._load (internal/modules/cjs/loader.js:778:12)
    ERR!     at Module.require (internal/modules/cjs/loader.js:961:19)
    ERR!     at require (internal/modules/cjs/helpers.js:92:18)
    ERR!     at interopRequireDefault (<path-to-project>/node_modules/@storybook/core-common/dist/cjs/utils/interpret-require.js:64:16)
    ERR!     at serverRequire (<path-to-project>/node_modules/@storybook/core-common/dist/cjs/utils/interpret-require.js:101:10)
    ERR!     at getPreviewBuilder (<path-to-project>/node_modules/@storybook/core-server/dist/cjs/utils/get-preview-builder.js:25:55)
    ERR!     at buildDevStandalone (<path-to-project>/node_modules/@storybook/core-server/dist/cjs/build-dev.js:99:71)
    ERR!     at async buildDev (<path-to-project>/node_modules/@storybook/core-server/dist/cjs/build-dev.js:154:5)
  5. Following those directions corrected the issue, so this PR is an attempt to do that automatically

What I did

  1. Attempt to default to commonJs: true when ran in an ESM project (e.g. "type": "module")

Submitting a draft PR because I'm only ~80% sure this is the way to do that, or that it should be done in the first place).

How to test

No idea how to test sb init

  • 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?

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 Sep 28, 2021

Nx Cloud Report

CI ran the following commands for commit 4904adb. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Status Command
#000000 nx run-many --target=prepare --all --parallel --max-parallel=15

Sent with 💌 from NxCloud.

@kylegach
Copy link
Contributor Author

kylegach commented Sep 28, 2021

After writing that description, I had a thought and noticed the readPackageJson utility in that file. Perhaps instead of always defaulting to commonJs: true for "web components" projects, we could read the type do that whenever type === 'module'?

Edit: I decided this was a better approach, so I just pushed a commit reverting the previous commit's changes and attempting this method. I've updated the title and description accordingly.

@kylegach kylegach changed the title Default sb init w/ "web components" to use cjs Default sb init in ESM projects to use cjs Sep 28, 2021
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

LGTM!

@kylegach kylegach marked this pull request as ready for review September 30, 2021 19:00
@kylegach kylegach merged commit ec16409 into next Sep 30, 2021
@kylegach kylegach deleted the sb-init-web-components-cjs branch September 30, 2021 19:01
@shilman shilman added this to the 6.4 PRs milestone Oct 25, 2021
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.

None yet

2 participants