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: Avoid framework imports from core/client #17875

Merged
merged 2 commits into from Apr 6, 2022
Merged

Core: Avoid framework imports from core/client #17875

merged 2 commits into from Apr 6, 2022

Conversation

IanVS
Copy link
Member

@IanVS IanVS commented Apr 4, 2022

Issue:

What I did

I found that the vite builder needed to pre-bundle some of the storybook dependencies, even though they distribute ESM. One cause was the framework packages importing directly from @storybook/core/client, which is a cjs file that just re-exports the package index.

This change allows imports of either cjs or esm in the normal fashion.

This may not be strictly necessary after https://github.com/storybookjs/storybook/pull/17868/files#diff-ed67a6365c9248d5f4412c4c904d2e3789155a763f681f8c48a63af38ba8a0f2R1 is merged, but it does seem strange to use this file to force one or the other packages to be used, rather than allowing normal node/bundler behavior.

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?

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

@IanVS IanVS added the bug label Apr 4, 2022
@IanVS IanVS requested review from tmeasday and shilman April 4, 2022 12:36
@nx-cloud
Copy link

nx-cloud bot commented Apr 4, 2022

☁️ Nx Cloud Report

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

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.

This looks fine to me, though probably not needed after #17868

We will refactor all of this in 7.0

@shilman shilman added maintenance User-facing maintenance tasks and removed bug labels Apr 4, 2022
@IanVS
Copy link
Member Author

IanVS commented Apr 4, 2022

I added a comment to lib/core/client.js based on https://github.com/storybookjs/storybook/pull/17868/files#r841762909. Do you have a convention for comments to identify cleanups to make in 7.0?

@IanVS IanVS changed the title Avoid forcing CJS import from core/client Avoid imports from core/client Apr 5, 2022
@IanVS
Copy link
Member Author

IanVS commented Apr 6, 2022

@shilman turns out this is still necessary, because the changes in #17868 did not move the lib/core/client.js file itself to an ES module (it still contains module.exports = . Mind if we merge this?

@tmeasday
Copy link
Member

tmeasday commented Apr 6, 2022

@IanVS lets fix that?

@IanVS
Copy link
Member Author

IanVS commented Apr 6, 2022

@tmeasday what do you have in mind? If cjs is still required for ie11 support, then I think we would need a multiple files, client.js and client.mjs perhaps? And an exports field in package.json? I'm not sure all that is worth it if this really is just a file for compatibility, and we can achieve the same results by not using it at all, right? Or maybe I'm misunderstanding.

@shilman shilman changed the title Avoid imports from core/client Core: Avoid framework imports from core/client Apr 6, 2022
@shilman
Copy link
Member

shilman commented Apr 6, 2022

@IanVS @tmeasday merging this now to get things unstuck and we can follow up in other PRs if needed

@shilman shilman merged commit 5698bd5 into next Apr 6, 2022
@shilman shilman deleted the core-client branch April 6, 2022 15:52
@tmeasday
Copy link
Member

tmeasday commented Apr 7, 2022

@IanVS that's not quite it:

If cjs is still required for ie11 support,

IE11 will consume a module-less bundle created by webpack, which will happily consume ESM.

CJS support in browser files is really just for storyshots and other Jest contexts. Which begs the question, why didn't my change break storyshots?

@IanVS
Copy link
Member Author

IanVS commented Apr 14, 2022

Can this be cherry-picked into the 6.4 branch? It is causing an issue in the vite builder: #17504, due to the framework package importing from the cjs version of core-client, whereas the esm version is used when we import from @storybook/client-api.

@shilman shilman added patch:yes Bugfix & documentation PR that need to be picked to main branch patch:done Patch/release PRs already cherry-picked to main/release branch labels Apr 14, 2022
shilman added a commit that referenced this pull request Apr 14, 2022
Core: Avoid framework imports from core/client
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance User-facing maintenance tasks patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants