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

fix: maintain compatibility with rollup when loading commonjs module #15600

Conversation

XiSenao
Copy link
Collaborator

@XiSenao XiSenao commented Jan 14, 2024

Description

fix #15542

Additional context

From the following repository examples:
https://github.com/XiSenao/exceptional-performance-of-loading-commonjs-modules/blob/main/src/main.js

it can be found that using namespace imports to load CommonJS modules results in inconsistent development and build behaviors. In most cases, dynamic import behaves as expected. That is to say, it seems effective to rewrite in the following way.

import('${rewrittenUrl}').then(m => m.default && m.default.__esModule ? m.default : ({ ...m.default, default: m.default }))

However, when exploring how the @rollup/plugin-commonjs plugin interacts with rollup, it seems that rollup will merge the transpiled ESM product through

mergeNamespaces({
  __proto__: null,
  default: getDefaultExportFromCjs(rectClamp)
}, [rectClamp])

The logic of the rollup processing and the rewritten dynamic import are somewhat similar, but it can be found that there are still boundary scenarios that will cause inconsistencies between development and build behaviors for dynamic imports(e.g. only export Array).


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines, especially the Pull Request Guidelines.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Update the corresponding documentation if needed.
  • Ideally, include relevant tests that fail without this PR but pass with it.

Copy link

stackblitz bot commented Jan 14, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@XiSenao XiSenao force-pushed the chore/load-commonjs-modules-to-maintain-compatibility-with-rollup branch from 1106283 to 6142211 Compare January 14, 2024 06:49
@XiSenao XiSenao changed the title [WIP]: Maintain compatibility with rollup when loading commonjs module [WIP]: fix: maintain compatibility with rollup when loading commonjs module Jan 14, 2024
@XiSenao XiSenao force-pushed the chore/load-commonjs-modules-to-maintain-compatibility-with-rollup branch 3 times, most recently from 9a6a65c to b207f75 Compare January 14, 2024 10:26
@XiSenao XiSenao force-pushed the chore/load-commonjs-modules-to-maintain-compatibility-with-rollup branch from b207f75 to 2c5dd68 Compare January 14, 2024 12:04
@XiSenao XiSenao changed the title [WIP]: fix: maintain compatibility with rollup when loading commonjs module fix: maintain compatibility with rollup when loading commonjs module Jan 15, 2024
@XiSenao XiSenao closed this Jan 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

import star works in dev but not in prod
1 participant