-
-
Notifications
You must be signed in to change notification settings - Fork 567
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(commonjs): support CJS modules re-exporting transpiled ESM modules #1165
Merged
lukastaegert
merged 34 commits into
rollup:commonjs/strict-require-order
from
fwouts:cjs-reexport
Apr 24, 2022
Merged
Changes from 31 commits
Commits
Show all changes
34 commits
Select commit
Hold shift + click to select a range
f829b36
fix(commonjs): attach correct plugin meta-data to commonjs modules
lukastaegert 1393c7c
feat(commonjs): reimplement dynamic import handling
lukastaegert 35770bd
feat(commonjs): add strictRequires option to wrap modules
lukastaegert 81628db
feat(commonjs): automatically wrap cyclic modules
lukastaegert 93cf5fb
feat(commonjs): Infer type for unidentified modules
lukastaegert 8d31f25
feat(commonjs): make namespace callable when requiring ESM with funct…
lukastaegert fb490c0
fix(commonjs): handle relative external imports
lukastaegert e672b42
feat(commonjs): limit ignoreTryCatch to external requires
lukastaegert 748f8ba
feat(commonjs): auto-detect conditional requires
lukastaegert c399755
feat(commonjs): add dynamicRequireRoot option
lukastaegert 4a84215
feat(commonjs): throw for dynamic requires from outside the configure…
lukastaegert 7e2691a
refactor(commonjs): deconflict helpers only once globals are known
lukastaegert a16a3cd
feat(commonjs): expose plugin version
lukastaegert 46776c6
fix(commonjs): do not transform "typeof exports" for mixed modules
lukastaegert 373684d
fix(commonjs): inject module name into dynamic require function
lukastaegert b7e0d7e
fix(commonjs): validate node-resolve peer version
lukastaegert 45a3141
fix(commonjs): use correct version and add package exports
lukastaegert 12dbaa3
fix(commonjs): proxy all entries to not break legacy polyfill plugins
lukastaegert 0fb0303
fix(commonjs): add heuristic to deoptimize requires after calling imp…
lukastaegert ebc1cb1
fix(commonjs): make sure type changes of esm imports are tracked
lukastaegert b61845f
fix(commonjs): handle external dependencies when using the cache
lukastaegert b1110d2
fix(commonjs): Do not change semantics when removing requires in if s…
lukastaegert f509b1a
fix(commonjs): Allow to dynamically require an empty file
lukastaegert 347c148
Add a test to illustrate problematic re-exported module
fwouts 2749ea9
fix(commonjs): support CJS modules re-exporting ESM modules
fwouts d637611
fix(commonjs): Warn when plugins do not pass options to resolveId
lukastaegert 095491b
Merge remote-tracking branch 'upstream/commonjs/strict-require-order'…
fwouts e3b4b0c
Update snapshots post-merge
fwouts c77baef
Update tests
fwouts 4863298
Update snapshot
fwouts 159336a
Remove unnecessary fixtures
fwouts 981918d
Remove comment given other tests do exactly the same
fwouts 205b187
Update snapshots
lukastaegert 224f904
Merge remote-tracking branch 'upstream/commonjs/strict-require-order'…
lukastaegert File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
3 changes: 3 additions & 0 deletions
3
packages/commonjs/test/fixtures/function/transpiled-esm-reexported-default/_config.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
module.exports = { | ||
description: 'creates the correct exports from CJS module re-exporting a transpiled ES module', | ||
}; |
2 changes: 2 additions & 0 deletions
2
packages/commonjs/test/fixtures/function/transpiled-esm-reexported-default/dep.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
Object.defineProperty(exports, '__esModule', { value: true }); | ||
exports.default = 'default'; |
3 changes: 3 additions & 0 deletions
3
packages/commonjs/test/fixtures/function/transpiled-esm-reexported-default/main.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
import dep from './proxy'; | ||
|
||
t.is(dep, 'default'); |
1 change: 1 addition & 0 deletions
1
packages/commonjs/test/fixtures/function/transpiled-esm-reexported-default/proxy.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
module.exports = require('./dep'); |
3 changes: 3 additions & 0 deletions
3
packages/commonjs/test/fixtures/function/transpiled-esm-reexported-entry-default/_config.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
module.exports = { | ||
description: 'creates the correct exports from CJS module re-exporting a transpiled ES module', | ||
}; |
2 changes: 2 additions & 0 deletions
2
packages/commonjs/test/fixtures/function/transpiled-esm-reexported-entry-default/entry.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
Object.defineProperty(exports, '__esModule', { value: true }); | ||
exports.default = 'default'; |
3 changes: 3 additions & 0 deletions
3
packages/commonjs/test/fixtures/function/transpiled-esm-reexported-entry-default/main.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
import * as entry from './proxy'; | ||
|
||
t.deepEqual(entry, { default: 'default' }); |
1 change: 1 addition & 0 deletions
1
packages/commonjs/test/fixtures/function/transpiled-esm-reexported-entry-default/proxy.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
module.exports = require('./entry'); |
3 changes: 3 additions & 0 deletions
3
packages/commonjs/test/fixtures/function/transpiled-esm-reexported-entry-mixed/_config.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
module.exports = { | ||
description: 'creates the correct exports from CJS module re-exporting a transpiled ES module', | ||
}; |
3 changes: 3 additions & 0 deletions
3
packages/commonjs/test/fixtures/function/transpiled-esm-reexported-entry-mixed/entry.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
Object.defineProperty(exports, '__esModule', { value: true }); | ||
exports.default = 'default'; | ||
exports.named = 'named'; |
3 changes: 3 additions & 0 deletions
3
packages/commonjs/test/fixtures/function/transpiled-esm-reexported-entry-mixed/main.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
import * as entry from './proxy'; | ||
|
||
t.deepEqual(entry, { default: 'default', named: 'named' }); |
1 change: 1 addition & 0 deletions
1
packages/commonjs/test/fixtures/function/transpiled-esm-reexported-entry-mixed/proxy.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
module.exports = require('./entry'); |
3 changes: 3 additions & 0 deletions
3
packages/commonjs/test/fixtures/function/transpiled-esm-reexported-entry-named/_config.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
module.exports = { | ||
description: 'creates the correct exports from CJS module re-exporting a transpiled ES module', | ||
}; |
2 changes: 2 additions & 0 deletions
2
packages/commonjs/test/fixtures/function/transpiled-esm-reexported-entry-named/entry.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
Object.defineProperty(exports, '__esModule', { value: true }); | ||
exports.named = 'named'; |
11 changes: 11 additions & 0 deletions
11
packages/commonjs/test/fixtures/function/transpiled-esm-reexported-entry-named/main.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
import * as entry from './proxy'; | ||
|
||
// Note: There ideally shouldn't be a default generated property. | ||
// | ||
// This should however be harmless. | ||
t.deepEqual(entry, { | ||
default: { | ||
named: 'named', | ||
}, | ||
named: 'named' | ||
}); |
1 change: 1 addition & 0 deletions
1
packages/commonjs/test/fixtures/function/transpiled-esm-reexported-entry-named/proxy.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
module.exports = require('./entry'); |
3 changes: 3 additions & 0 deletions
3
packages/commonjs/test/fixtures/function/transpiled-esm-reexported-mixed/_config.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
module.exports = { | ||
description: 'creates the correct exports from CJS module re-exporting a transpiled ES module', | ||
}; |
3 changes: 3 additions & 0 deletions
3
packages/commonjs/test/fixtures/function/transpiled-esm-reexported-mixed/dep.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
Object.defineProperty(exports, '__esModule', { value: true }); | ||
exports.named = 'named'; | ||
exports.default = 'default'; |
4 changes: 4 additions & 0 deletions
4
packages/commonjs/test/fixtures/function/transpiled-esm-reexported-mixed/main.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
import dep, { named } from './proxy'; | ||
|
||
t.is(dep, 'default'); | ||
t.is(named, 'named'); |
1 change: 1 addition & 0 deletions
1
packages/commonjs/test/fixtures/function/transpiled-esm-reexported-mixed/proxy.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
module.exports = require('./dep'); |
3 changes: 3 additions & 0 deletions
3
packages/commonjs/test/fixtures/function/transpiled-esm-reexported-named/_config.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
module.exports = { | ||
description: 'creates the correct exports from CJS module re-exporting a transpiled ES module', | ||
}; |
2 changes: 2 additions & 0 deletions
2
packages/commonjs/test/fixtures/function/transpiled-esm-reexported-named/dep.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
Object.defineProperty(exports, '__esModule', { value: true }); | ||
exports.named = 'named'; |
3 changes: 3 additions & 0 deletions
3
packages/commonjs/test/fixtures/function/transpiled-esm-reexported-named/main.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
import { named } from './proxy'; | ||
|
||
t.is(named, 'named'); |
1 change: 1 addition & 0 deletions
1
packages/commonjs/test/fixtures/function/transpiled-esm-reexported-named/proxy.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
module.exports = require('./dep'); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note this particular bug that the change introduces. I'm not sure it's possible to avoid it, but would love suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, will need to thing about this. In any case your observation
is not correct. Rollup can "look into a module before including it" vie
this.load
, and as a matter of fact, it already does extensively inresolve-require-sources
. So it should actually be possible to extend this logic to figure out e.g. if we want to create a default export I guess (currently we only know if we are requiring CJS or ES), but this will make things MUCH more complicated.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! That's useful info. I'd be willing to explore that if you prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, have a look, but the logic is slightly involved, feel free to ask questions. I guess the current logic will be good enough as well, but I will need to think a little more about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, still exploring and not quite understanding everything yet but I do have a question!
Is the definition of this test intentionally replacing the
module.exports
object—which has its__esModule
property set totrue
—with a string? Is the idea to have the static analysis kick in but have__esModule
missing at runtime?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After exploring a bit more, I found out that this spurious
default
export is already expected in other tests such as this one, so it's not actually new behaviour, and not something we need to worry about in the context of this PR.I think there is a more general refactor required, which like you suggested would look at the
meta
data collected in required modules and only generate a default export if we expect there to be one. This could even allow us to remove thegetDefaultExportFromCjs
helper entirely (it would always beexport default x["default"]
).I'm now of the opinion that this general refactor would be better done in a separate PR, to be implemented after #1038. It may also be alright to leave it unchanged :)
I propose to merge this PR as-is.