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
Do not export null as module.exports in amd.js. Resolves #2332. #2334
Conversation
File size impactMerging issue-2332 into master will impact 4 files in 3 groups. browser (2/2)
node (1/1)
extras (1/8)
|
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.
Nice work tracking this down. Let's perhaps just make the core code resiliant to null
here?
Since this is the problematic line (https://github.com/systemjs/systemjs/blob/master/src/system-core.js#L115):
if (name.__esModule)
we could just change that line to:
if (name && name.__esModule)
Done! Also added similar for named-exports.js |
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.
Looks Good
src/extras/amd.js
Outdated
// https://github.com/systemjs/systemjs/issues/2332 | ||
if (module.exports !== null) | ||
_export(module.exports); |
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.
With the core change we shouldn't need this null check anymore:
// https://github.com/systemjs/systemjs/issues/2332 | |
if (module.exports !== null) | |
_export(module.exports); |
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.
👍🏿 updated
See #2332 - both system-core and named-exports do not work if
_export(null)
occurs, as it appears to be invalid. However, amd.js would do this as explained in #2332. My fix here is specific only to null, rather than an object check + null check, since it's valid to loop over the keys of functions for re-exporting.