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

transformMixedEsModules shouldn't replace typeof exports #1014

Closed
gluck opened this issue Oct 7, 2021 · 10 comments · Fixed by #1038
Closed

transformMixedEsModules shouldn't replace typeof exports #1014

gluck opened this issue Oct 7, 2021 · 10 comments · Fixed by #1038

Comments

@gluck
Copy link

gluck commented Oct 7, 2021

  • Rollup Plugin Name: @rollup/plugin-commonjs
  • Rollup Plugin Version: 21.0.0

Expected Behavior / Situation

If an ESM file is transformed with transformMixedEsModules=true (for require support), it shouldn't replace typeof exports by 'object' and leave it as-is (or replace by 'undefined' ?)

Actual Behavior / Situation

Right now the bundle fails with exports is not defined because of code in ESM such as lodash-es:
https://github.com/lodash/lodash/blob/es/isBuffer.js#L5

var freeExports = typeof exports == 'object' && exports && !exports.nodeType && exports;
// becomes:
var freeExports = exports && !exports.nodeType && exports;

(lodash-es itself doesn't require transformMixedEsModules, but other parts of the bundle do)

Modification Proposal

If esModule=true do not transform typeof module/exports/module.exports into 'object', as they'll likely open invalid code paths in an ESM.

(I can PR with tests if agreed)

@rollup rollup deleted a comment from fi3ework Nov 17, 2021
@yunsii
Copy link

yunsii commented Dec 8, 2021

Any progress? lodash-es cause the same error, when I use Vite with transformMixedEsModules=true.

@lukastaegert
Copy link
Member

I added a fix to #1038

@yunsii
Copy link

yunsii commented Dec 17, 2021

I added a fix to #1038

Hello, when will it be released?

@shellscape
Copy link
Collaborator

@theprimone we don't have a release schedule. please exercise patience.

@yunsii
Copy link

yunsii commented Dec 17, 2021

@theprimone we don't have a release schedule. please exercise patience.

Well,how can I make a workaround?It causes error after bundled, though I try to configure exclude.

@yunsii
Copy link

yunsii commented Dec 19, 2021

It looks like version @rollup/plugin-commonjs@22.0.0-4 fixes the problem for now, detail: https://unpkg.com/browse/@rollup/plugin-commonjs@22.0.0-4/dist/index.es.js

@webhao
Copy link

webhao commented Feb 10, 2022

@yunsii I didn't solve it.update@rollup/plugin-commonjs@22.0.0-9,use commonjsOptions: { transformMixedEsModules: true, strictRequires: true, }

@yunsii
Copy link

yunsii commented Feb 10, 2022

@yunsii I didn't solve it.update@rollup/plugin-commonjs@22.0.0-9,use commonjsOptions: { transformMixedEsModules: true, strictRequires: true, }

The same error: exports is not defined? I replace require with import ... from ... and remove commonjs plugin finnally 😂

@mccxiv
Copy link

mccxiv commented Feb 24, 2022

Workaround while waiting for the fix to release:

Add window.exports = undefined before anything else executes.
For me, I had to do it in a separate script tag above the main file:

<script>
  // Workaround for https://github.com/rollup/plugins/issues/1014
  window.exports = undefined
</script>

@superchangme
Copy link

help me vite 2.6.8 still has this problem Uncaught ReferenceError: exports is not defined

lukastaegert added a commit that referenced this issue Apr 24, 2022
#1165)

* fix(commonjs): attach correct plugin meta-data to commonjs modules

* feat(commonjs): reimplement dynamic import handling

BREAKING CHANGES: requires Node 12
No longer supports require.cache

* feat(commonjs): add strictRequires option to wrap modules

* feat(commonjs): automatically wrap cyclic modules

* feat(commonjs): Infer type for unidentified modules

* feat(commonjs): make namespace callable when requiring ESM with function default

* fix(commonjs): handle relative external imports

* feat(commonjs): limit ignoreTryCatch to external requires

* feat(commonjs): auto-detect conditional requires

* feat(commonjs): add dynamicRequireRoot option

* feat(commonjs): throw for dynamic requires from outside the configured root

* refactor(commonjs): deconflict helpers only once globals are known

* feat(commonjs): expose plugin version

* fix(commonjs): do not transform "typeof exports" for mixed modules

resolves #1014

* fix(commonjs): inject module name into dynamic require function

* fix(commonjs): validate node-resolve peer version

* fix(commonjs): use correct version and add package exports

* fix(commonjs): proxy all entries to not break legacy polyfill plugins

* fix(commonjs): add heuristic to deoptimize requires after calling imported function

* fix(commonjs): make sure type changes of esm imports are tracked

BREAKING CHANGES: Requires at least rollup@2.68.0

* fix(commonjs): handle external dependencies when using the cache

* fix(commonjs): Do not change semantics when removing requires in if statements

* fix(commonjs): Allow to dynamically require an empty file

* Add a test to illustrate problematic re-exported module

This is exhibited for example in Next.js, see https://github.com/vercel/next.js/blob/5feb400aff8e7b8968174b4e339b98ce48412180/packages/next/link.js#L1 which doesn't specify __esModule but re-exports a module that does.

See https://www.runpkg.com/?next@12.1.4/link.js and https://www.runpkg.com/?next@12.1.4/dist/client/link.js for the compiled example.

* fix(commonjs): support CJS modules re-exporting ESM modules

* fix(commonjs): Warn when plugins do not pass options to resolveId

* Update snapshots post-merge

* Update tests

* Update snapshot

* Remove unnecessary fixtures

* Remove comment given other tests do exactly the same

See:
- https://github.com/rollup/plugins/blob/d637611a79a2701c359f5f2f6ffb49978070da38/packages/commonjs/test/fixtures/function/transpiled-esm-entry-named/main.js#L3
- https://github.com/rollup/plugins/blob/d637611a79a2701c359f5f2f6ffb49978070da38/packages/commonjs/test/fixtures/function/transpiled-esm-namespace-named/main.js#L5

* Update snapshots

Co-authored-by: Lukas Taegert-Atkinson <lukas.taegert-atkinson@tngtech.com>
lukastaegert added a commit that referenced this issue Apr 24, 2022
#1165)

* fix(commonjs): attach correct plugin meta-data to commonjs modules

* feat(commonjs): reimplement dynamic import handling

BREAKING CHANGES: requires Node 12
No longer supports require.cache

* feat(commonjs): add strictRequires option to wrap modules

* feat(commonjs): automatically wrap cyclic modules

* feat(commonjs): Infer type for unidentified modules

* feat(commonjs): make namespace callable when requiring ESM with function default

* fix(commonjs): handle relative external imports

* feat(commonjs): limit ignoreTryCatch to external requires

* feat(commonjs): auto-detect conditional requires

* feat(commonjs): add dynamicRequireRoot option

* feat(commonjs): throw for dynamic requires from outside the configured root

* refactor(commonjs): deconflict helpers only once globals are known

* feat(commonjs): expose plugin version

* fix(commonjs): do not transform "typeof exports" for mixed modules

resolves #1014

* fix(commonjs): inject module name into dynamic require function

* fix(commonjs): validate node-resolve peer version

* fix(commonjs): use correct version and add package exports

* fix(commonjs): proxy all entries to not break legacy polyfill plugins

* fix(commonjs): add heuristic to deoptimize requires after calling imported function

* fix(commonjs): make sure type changes of esm imports are tracked

BREAKING CHANGES: Requires at least rollup@2.68.0

* fix(commonjs): handle external dependencies when using the cache

* fix(commonjs): Do not change semantics when removing requires in if statements

* fix(commonjs): Allow to dynamically require an empty file

* Add a test to illustrate problematic re-exported module

This is exhibited for example in Next.js, see https://github.com/vercel/next.js/blob/5feb400aff8e7b8968174b4e339b98ce48412180/packages/next/link.js#L1 which doesn't specify __esModule but re-exports a module that does.

See https://www.runpkg.com/?next@12.1.4/link.js and https://www.runpkg.com/?next@12.1.4/dist/client/link.js for the compiled example.

* fix(commonjs): support CJS modules re-exporting ESM modules

* fix(commonjs): Warn when plugins do not pass options to resolveId

* Update snapshots post-merge

* Update tests

* Update snapshot

* Remove unnecessary fixtures

* Remove comment given other tests do exactly the same

See:
- https://github.com/rollup/plugins/blob/d637611a79a2701c359f5f2f6ffb49978070da38/packages/commonjs/test/fixtures/function/transpiled-esm-entry-named/main.js#L3
- https://github.com/rollup/plugins/blob/d637611a79a2701c359f5f2f6ffb49978070da38/packages/commonjs/test/fixtures/function/transpiled-esm-namespace-named/main.js#L5

* Update snapshots

Co-authored-by: Lukas Taegert-Atkinson <lukas.taegert-atkinson@tngtech.com>
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 a pull request may close this issue.

7 participants