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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Webpack5: Make export-order-loader compatible with both esm and cjs #25540

Merged
merged 3 commits into from
Jan 16, 2024

Conversation

mlazari
Copy link
Contributor

@mlazari mlazari commented Jan 10, 2024

Closes #25383

What I did

The new webpack loader added in #24749 assumes the code is ES (parses it with es-module-lexer to find the exports). Sometimes the code is CommonJS though, for example if @babel/plugin-transform-modules-commonjs is used directly or indirectly. In that case the loader doesn't find the exports and the source output is CommonJS mixed with ES which leads to an "exports is not defined" error.
This change will try to get the named exports with cjs-module-lexer if it can't find any exports or imports with es-module-lexer, and will add ;module.exports.__namedExportsOrder = ${namedExportsOrderString}; instead of ;export const __namedExportsOrder = ${namedExportsOrderString}; in that case.
This also includes the changes in #25506 to skip the loader if it fails for some reason, as it isn't crucial for Storybook to work properly.

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

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

I tested the change in mlazari/sbsandbox@c63f4fe
Output source of the loader with and without @babel/plugin-transform-modules-commonjs: https://github.com/mlazari/sbsandbox/blob/main/loader-changes.md

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-25540-sha-c0037961. Try it out in a new sandbox by running npx storybook@0.0.0-pr-25540-sha-c0037961 sandbox or in an existing project with npx storybook@0.0.0-pr-25540-sha-c0037961 upgrade.

More information
Published version 0.0.0-pr-25540-sha-c0037961
Triggered by @valentinpalkovic
Repository mlazari/storybook
Branch next
Commit c0037961
Datetime Tue Jan 16 12:07:31 UTC 2024 (1705406851)
Workflow run 7541327976

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=25540

@valentinpalkovic valentinpalkovic self-assigned this Jan 10, 2024
@valentinpalkovic valentinpalkovic added bug patch:yes Bugfix & documentation PR that need to be picked to main branch builder-webpack5 ci:daily Run the CI jobs that normally run in the daily job. labels Jan 10, 2024
@valentinpalkovic
Copy link
Contributor

valentinpalkovic commented Jan 10, 2024

@mlazari
Well done! Amazing work.

I did a minor refactoring to make the code more readable and to fix type issues.

@valentinpalkovic valentinpalkovic changed the title Make export-order-loader compatible with both es and cjs Webpack5: Make export-order-loader compatible with both esm and cjs Jan 16, 2024
@valentinpalkovic valentinpalkovic merged commit dfb08da into storybookjs:next Jan 16, 2024
88 of 89 checks passed
@github-actions github-actions bot mentioned this pull request Jan 16, 2024
5 tasks
JReinhold pushed a commit that referenced this pull request Jan 16, 2024
Webpack5: Make export-order-loader compatible with both esm and cjs
(cherry picked from commit dfb08da)
@github-actions github-actions bot added the patch:done Patch/release PRs already cherry-picked to main/release branch label Jan 17, 2024
@github-actions github-actions bot mentioned this pull request Jan 17, 2024
21 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug builder-webpack5 ci:daily Run the CI jobs that normally run in the daily job. 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.

[Bug]: ReferenceError: exports is not defined after update to 7.6
2 participants