-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Improve output for re-exporting #16178
base: main
Are you sure you want to change the base?
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/56469 |
28daa27
to
8abdadf
Compare
22fa614
to
b8c7d64
Compare
Object.defineProperty(exports, name, { | ||
enumerable: true, | ||
get: function () { | ||
return mod[name2 || name]; |
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.
This fails if name2
is the empty string:
export { "" as x } from "./mod"
if (key in exports && exports[key] === _foo[key]) return; | ||
exports[key] = _foo[key]; | ||
}); | ||
__exportStar(require("foo")); |
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.
Does this still work the same with cjs-module-lexer?
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.
Yes, this is pattern detection for ts by cjs-module-lexer
.
var _dep = babelHelpers.interopRequireWildcard(require("dep"), true); | ||
_export("default", _dep); | ||
_export("name", _dep); | ||
function _export(name, mod, name2) { |
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.
Could we move this to an helper?
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.
This function uses exports
unless we pass it as a parameter.
var _react = __exportStar(require("react")); | ||
var _interop; | ||
function __exportStar(mod) { | ||
mod = _interop == 1 ? babelHelpers.interopRequireWildcard(mod) : mod; |
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.
Maybe we could move the interopRequireWildcard call to __exportStar(_interopRequireWildcard(require("react"))
, so that we don't need the _interop
variable? If not, in cases where _interop
always has the same value could we hard-code the correct branch of this ternary rather than leaving both?
var _foo = require("foo"); | ||
_export("bar", _foo); |
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.
How do these changes change the case of a cycle, where foo
is importing bar
from this file?
// output.js
export { bar } from "foo"
// foo
import { bar as b } from "output.js"
export const bar = 1;
console.log(bar);
and
// output.js
export { bar } from "foo"
// foo
import { bar as b } from "output.js"
console.log(bar);
export const bar = 1;
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.
I added these two tests and strangely there is no change in behavior. I'm surprised.🤔
Thank you for your review! I'm marking it as draft for now to try and unify the behavior of |
b8c7d64
to
7479972
Compare
The new behavior is compatible with
cjs-module-lexer
viaexports.x=void 0
.When the user has enabled the
lazy
option or the oldbabel-plugin-transform-modules-commonjs
is working with the newbabel-helper-module-transforms
,implicitAssignmentExports
will beundefined
to follow the old behavior .Note that there is an observable behavioral difference here.
Named exports are now no longer
configurable: false
.