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

Core: Rename generated-stories-entry to cjs extension so require works #16727

Merged
merged 9 commits into from Jan 24, 2022
Merged

Core: Rename generated-stories-entry to cjs extension so require works #16727

merged 9 commits into from Jan 24, 2022

Conversation

jpzwarte
Copy link
Member

Possible fix for #14877.

@nx-cloud
Copy link

nx-cloud bot commented Nov 19, 2021

☁️ Nx Cloud Report

CI ran the following commands for commit 07ca419. 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.

Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

@jpzwarte I am surprised you don't need to make the same adjustment to the xxx-generated-config-entry.js also? (lines 128 & 136 above?).

@jpzwarte
Copy link
Member Author

@tmeasday I've added them, but afaics they're not really necessary somehow.

Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

LGTM. I guess we may as well use .cjs everywhere we are going to compiling templates to require() code.

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.

@tmeasday this seems to break angular somehow per the e2e tests

@tmeasday
Copy link
Member

tmeasday commented Dec 6, 2021

Ok, so the reason this breaks is that the virtualModuleEntry.template.js is not compiled to CJS (in the dist/cjs directory 😕 ), because of these line (I think?)

storybook/.babelrc.js

Lines 139 to 156 in 8ea0e35

{
test: ['**/virtualModuleEntry.template.js'],
presets: [
[
'@babel/preset-env',
{
shippedProposals: true,
useBuiltIns: 'usage',
targets: {
node: '10',
},
corejs: '3',
modules: false,
},
],
],
},
],

@shilman git blame isn't helping me much here. Do you recall why this is here (seems a bit weird, but I don't particularly want to change it). I guess we have 3 options:

  1. Actually compile the virtualModuleEntry.template.js file to CJS in the dist/cjs folder. This seems like the logical thing to do but I guess maybe it is like this for a reason.

  2. Also not compile virtualModuleStory.template.js to CJS in dist/cjs. This would be consistent with the other file, and possibly fix the original problem 🤷

  3. Just use .js for the first and .cjs for the second, with a comment explaining why.

I would lean towards 3 for reasons of caution I guess.

@jpzwarte I guess this is why you didn't originally need to change the module file.

@shilman
Copy link
Member

shilman commented Dec 6, 2021

@tmeasday I think this is a question for @ndelangen

@psimk psimk mentioned this pull request Dec 6, 2021
11 tasks
@tmeasday
Copy link
Member

@ndelangen can I get you take a quick look at my comment above? If it doesn't ring any bells, we'll just take my option 3 and be done with it. Sorry for the delays @jpzwarte!

@ndelangen
Copy link
Member

Let's do option 3, and revisit this for 7.0?

@tmeasday
Copy link
Member

@jpzwarte any appetite for reverting what I asked you to do and adding a comment explaining why? 🙏

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! Thanks @jpzwarte @tmeasday 🚀

@shilman shilman changed the title Rename to .cjs extension so require works. Core: Rename generated-stories-entry to cjs extension so require works Jan 24, 2022
@shilman shilman merged commit ee31c37 into storybookjs:next Jan 24, 2022
@jpzwarte
Copy link
Member Author

Thanks for finishing this @shilman & @tmeasday ! Too busy to do it myself :/

@tmeasday
Copy link
Member

Totally understand @jpzwarte, no problem at all!

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

4 participants