Skip to content

Rollup ^3.0.0 cjs re-exports edge case with externalLiveBindings: false #79

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

Closed
yyx990803 opened this issue Jan 20, 2023 · 8 comments · Fixed by rollup/rollup#4826
Closed

Comments

@yyx990803
Copy link

yyx990803 commented Jan 20, 2023

This is a new version of the same problem reported back in #38

Given code

export * from 'foo'

Default cjs output from Rollup (working, correctly detects re-exports from foo):

var foo = require('foo');

Object.keys(foo).forEach(function (k) {
	if (k !== 'default' && !exports.hasOwnProperty(k)) Object.defineProperty(exports, k, {
		enumerable: true,
		get: function () { return foo[k]; }
	});
});

With output.externalLiveBindings: false (not working, no re-exports detected):

var foo = require('foo');

for (var k in foo) {
	if (k !== 'default' && !exports.hasOwnProperty(k)) exports[k] = foo[k];
}

Note: this was working previously in Rollup 2.x where the code generated was:

var foo = require('foo');

Object.keys(foo).forEach(function (k) {
	if (k !== 'default' && !exports.hasOwnProperty(k)) exports[k] = foo[k];
});

Reproduction

  • Install deps
  • npm run build (which builds src/index.js with Rollup to out.js)
  • node lex.js (which runs cjs-module-lexer on out.js)
@guybedford
Copy link
Collaborator

Thanks for reporting. Per the policy on this project, the patterns supported were fully stabilized at the point this library started shipping, so that no new detections will be added. Otherwise that risks code that works in new Node.js versions mysteriously breaking in older versions and that would be very very difficult to debug.

I've posted a PR in #80 to better explain this as part of the project status.

@yyx990803
Copy link
Author

/cc @lukastaegert

If Node.js will no longer update the detection mechanism, maybe Rollup will have to adapt to this case? I think it comes down to the use of for ... in vs Object.keys(...).forEach(...).

@guybedford
Copy link
Collaborator

Unfortunately that basically is the situation today - where build tools do have to accept these semantics as part of their targets now. It's ended up the lesser of the evils, and my apologies we couldn't do better!

@guybedford
Copy link
Collaborator

esbuild ended up just using a hinting pattern itself - #46

@lukastaegert
Copy link

I think the change was done to allow some level of improved ES3 compatibility via this option. But I see that this is only affecting a very minor group of users and since .forEach can easily be polyfilled, I will revert it to the other pattern.

@lukastaegert
Copy link

Actually, looking at this longer, it was not about ES3 compatibility. I wanted to switch to the other pattern altogether because it is smaller and more performant, but it cannot be used for live bindings because var k is being reassigned. If you allow const, then it will also use a for..in loop with live-bindings: https://rollupjs.org/repl/?version=3.11.0&shareable=JTdCJTIyZXhhbXBsZSUyMiUzQSUyMiUyMiUyQyUyMm1vZHVsZXMlMjIlM0ElNUIlN0IlMjJjb2RlJTIyJTNBJTIyZXhwb3J0JTIwKiUyMGZyb20lMjAnZm9vJyUyMiUyQyUyMmlzRW50cnklMjIlM0F0cnVlJTJDJTIybmFtZSUyMiUzQSUyMm1haW4uanMlMjIlN0QlNUQlMkMlMjJvcHRpb25zJTIyJTNBJTdCJTIyb3V0cHV0JTIyJTNBJTdCJTIyZm9ybWF0JTIyJTNBJTIyY2pzJTIyJTJDJTIyZ2VuZXJhdGVkQ29kZSUyMiUzQSU3QiUyMmNvbnN0QmluZGluZ3MlMjIlM0F0cnVlJTdEJTdEJTdEJTdE

But yes, I will revert this change.

@lukastaegert
Copy link

Fix at rollup/rollup#4826

@yyx990803
Copy link
Author

Thanks @lukastaegert - I will close this one now.

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.

3 participants