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: Fix addon bundling script #26145

Merged
merged 2 commits into from
Feb 23, 2024
Merged

Core: Fix addon bundling script #26145

merged 2 commits into from
Feb 23, 2024

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen commented Feb 22, 2024

Closes: #26126
Closes: Jakob-em/storybook-addon-angular-router#26
Closes: #26030

What I did

I researched the problem and discovered:

  • this is only a problem when using webpack
  • our addons/actions/dist/index.js (which is an export-type-entry for the addon is compiled in ESM and CJS, however the CJS is compiled for NODE.
  • our addons/actions/dist/index.js (which is an import-type-entry for the addon is compiled in ESM and CJS, however the CJS is compiling in all preview related packages (package consolidation would fix this).
  • The user's config is compiling stories to CJS, which then causes the CJS version of the export-type-entry to be chosen by webpack.

These factors cause code designed to be run in NODE (unbundled), to be bundled again and then ran in a browser.

It seems webpack just can't really work with the output from esbuild when esbuild compiled for NODE, and webpack compiles for the browser. It creates an empty context, which then causes the issue.

How I fixed it:

I changed the bundling script that bundles our addons, so that it now does:

  • always externalizes all the storybook packages, including preview-api.
  • it compiles for a platform of neutral.
  • it choses whatever entry it deems best, no longer forcing it to use module.

The result is that:

  • 7.x repro works with this change applied
  • after upgrading the 7.x repro to sb8 beta, and applying this fix, it works
  • The dist output of addons/actions/dist/index.js has 7800 lines of code removed.

What is next?

I am pretty confident in this change, however it does change the dynamics with NODE.

I'd like to ask @yannbf 's help to verify if this did not change/break portable stories or the test-runner in some way.

I'd also like @shilman 's position on how safe they feel this change is.

This change would need to be patched back to 7.x.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

I published a canary, so testing this with any sandbox or repro should be easy.

Check if:

  • upgrade works, storybook starts
  • if all expected addon panels show up
  • if addon-a11y functionality works
  • if addon-actions functionality

Also check:

  • does the test-runner still works.
  • does portable stories work as expected

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts

  • Make sure this PR contains one of the labels below:

    Available labels
    • bug: Internal changes that fixes incorrect behavior.
    • maintenance: User-facing maintenance tasks.
    • dependencies: Upgrading (sometimes downgrading) dependencies.
    • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
    • cleanup: Minor cleanup style change. Will not show up in release changelog.
    • documentation: Documentation only changes. Will not show up in release changelog.
    • feature request: Introducing a new feature.
    • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
    • other: Changes that don't fit in the above categories.

🦋 Canary release

This pull request has been released as version 0.0.0-pr-26145-sha-32b8346f. Try it out in a new sandbox by running npx storybook@0.0.0-pr-26145-sha-32b8346f sandbox or in an existing project with npx storybook@0.0.0-pr-26145-sha-32b8346f upgrade.

More information
Published version 0.0.0-pr-26145-sha-32b8346f
Triggered by @ndelangen
Repository storybookjs/storybook
Branch norbert/fix-addon-bundling
Commit 32b8346f
Datetime Thu Feb 22 12:30:52 UTC 2024 (1708605052)
Workflow run 8004494946

To request a new release of this pull request, mention the @storybookjs/core team.

core team members can create a new canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=26145

…s even for CJS output, make CJS output neutral platform
@ndelangen ndelangen changed the title fix the addon bundling script so it externalizes all for index entries even for CJS output, make CJS output neutral platform Core: Fix addon bundling script Feb 22, 2024
@ndelangen ndelangen self-assigned this Feb 22, 2024
@ndelangen ndelangen added api: addons patch:yes Bugfix & documentation PR that need to be picked to main branch core ci:daily Run the CI jobs that normally run in the daily job. labels Feb 22, 2024
@ndelangen ndelangen added the bug label Feb 22, 2024
Co-authored-by: Jeppe Reinhold <jeppe@chromatic.com>
@ndelangen ndelangen marked this pull request as ready for review February 22, 2024 12:49
@yannbf
Copy link
Member

yannbf commented Feb 22, 2024

I used the canary to test portable stories and although I found issues, these issues are unrelated to this PR, so from the perspective of portable-stories I think the changes look good 👍

Copy link
Member

@yannbf yannbf 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 Think!

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.

I'm not confident about patching this back. But let's get it out in 8.0-beta ASAP and see how it does there. Great job!!! ❤️

@ndelangen ndelangen merged commit 21a2300 into next Feb 23, 2024
110 of 112 checks passed
@ndelangen ndelangen deleted the norbert/fix-addon-bundling branch February 23, 2024 08:10
@github-actions github-actions bot mentioned this pull request Mar 12, 2024
11 tasks
@AndyOGo
Copy link

AndyOGo commented Mar 14, 2024

Will this land in v7 any time soon?

shilman pushed a commit that referenced this pull request Mar 17, 2024
Core: Fix addon bundling script
(cherry picked from commit 21a2300)
@github-actions github-actions bot added the patch:done Patch/release PRs already cherry-picked to main/release branch label Mar 18, 2024
@yannbf
Copy link
Member

yannbf commented Mar 26, 2024

Will this land in v7 any time soon?

This is included in 7.6.18 and 8.0.0

@AndyOGo
Copy link

AndyOGo commented Mar 26, 2024

@yannbf
Am I looking at the wrong package?

This is included in 7.6.18 and 8.0.0

Both of these aren't available:

Screenshot 2024-03-26 at 19 08 16

@AndyOGo
Copy link

AndyOGo commented Mar 28, 2024

@ndelangen
@yannbf
@shilman
@JReinhold

This is included in 7.6.18 and 8.0.0

Which package please?

These aren't published as v7.6.18

@shilman
Copy link
Member

shilman commented Mar 29, 2024

@AndyOGo It doesn't look like 7.6.18 has been published. Are you unable to upgrade to Storybook 8?

@AndyOGo
Copy link

AndyOGo commented Mar 29, 2024

@shilman Thx for your answer.
Yes, we can't upgrade to v8 at the moment.

@sjwilczynski
Copy link
Contributor

For us, it is also a problem. Would really appreciate if it could be ported back to v7

@reshu0408
Copy link

reshu0408 commented Apr 6, 2024

@shilman @yannbf
Its causing a problem for us too. and blocking an important release. Can you please work to get the patch in V7 version. Please. Thankyou.

@yannbf
Copy link
Member

yannbf commented Apr 8, 2024

@ndelangen seems like the patch release that was going to include this change was closed, I'm not sure why: #26121

@sjwilczynski
Copy link
Contributor

@yannbf, @ndelangen any chances of this being release in 7.x?

@heathercodes
Copy link

+1 when will this be released in 7.6.18? we can't jump all the way to v8 yet, we need a working v7 first.

@AndyOGo
Copy link

AndyOGo commented Apr 21, 2024

@ndelangen Please respond

JReinhold pushed a commit that referenced this pull request Apr 22, 2024
Core: Fix addon bundling script
(cherry picked from commit 21a2300)
@JReinhold JReinhold mentioned this pull request Apr 23, 2024
1 task
@JReinhold
Copy link
Contributor

@AndyOGo, @heathercodes, @sjwilczynski, @reshu0408 v7.6.18 has been released now with the fix.

https://github.com/storybookjs/storybook/releases/tag/v7.6.18

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: addons bug ci:daily Run the CI jobs that normally run in the daily job. core 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
8 participants