Skip to content

Improve output for re-exporting#16178

Open
liuxingbaoyu wants to merge 3 commits intobabel:mainfrom
liuxingbaoyu:cjs-exports
Open

Improve output for re-exporting#16178
liuxingbaoyu wants to merge 3 commits intobabel:mainfrom
liuxingbaoyu:cjs-exports

Conversation

@liuxingbaoyu
Copy link
Copy Markdown
Member

@liuxingbaoyu liuxingbaoyu commented Dec 14, 2023

Q                       A
Fixed Issues? Closes #15709
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

The new behavior is compatible with cjs-module-lexer via exports.x=void 0.
When the user has enabled the lazy option or the old babel-plugin-transform-modules-commonjs is working with the new babel-helper-module-transforms, implicitAssignmentExports will be undefined to follow the old behavior .

Note that there is an observable behavioral difference here.
Named exports are now no longer configurable: false.

@liuxingbaoyu liuxingbaoyu added the PR: Output optimization 🔬 A type of pull request used for our changelog categories label Dec 14, 2023
@babel-bot
Copy link
Copy Markdown
Collaborator

babel-bot commented Dec 14, 2023

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/59111

@liuxingbaoyu liuxingbaoyu marked this pull request as draft December 14, 2023 06:43
@liuxingbaoyu liuxingbaoyu force-pushed the cjs-exports branch 3 times, most recently from 28daa27 to 8abdadf Compare December 14, 2023 09:02
@liuxingbaoyu liuxingbaoyu marked this pull request as ready for review December 14, 2023 11:47
@JLHwung JLHwung self-requested a review January 25, 2024 20:50
Object.defineProperty(exports, name, {
enumerable: true,
get: function () {
return mod[name2 || name];
Copy link
Copy Markdown
Member

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"));
Copy link
Copy Markdown
Member

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?

Copy link
Copy Markdown
Member Author

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) {
Copy link
Copy Markdown
Member

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?

Copy link
Copy Markdown
Member Author

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;
Copy link
Copy Markdown
Member

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?

});
exports.foo = exports.bar = void 0;
var _foo = require("foo");
_export("bar", _foo);
Copy link
Copy Markdown
Member

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;

Copy link
Copy Markdown
Member Author

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.🤔

@liuxingbaoyu
Copy link
Copy Markdown
Member Author

Thank you for your review! I'm marking it as draft for now to try and unify the behavior of lazy.

@liuxingbaoyu liuxingbaoyu marked this pull request as ready for review April 22, 2025 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR: Output optimization 🔬 A type of pull request used for our changelog categories

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants