Skip to content
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

Overriding export * causes TypeError #3137

Closed
klaussner opened this issue Sep 26, 2019 · 7 comments · Fixed by #3959
Closed

Overriding export * causes TypeError #3137

klaussner opened this issue Sep 26, 2019 · 7 comments · Fixed by #3959

Comments

@klaussner
Copy link

  • Rollup Version: 1.21.4
  • Operating System (or Browser): macOS 10.14.6
  • Node Version: 12.2

How Do We Reproduce?

REPL link.

export * from "fs";
export const existsSync = ":)";

Expected Behavior

The second export overrides existsSync of the first export.

Actual Behavior

A TypeError is thrown when the Rollup output is executed in Node:

TypeError: Cannot set property existsSync of #<Object> which has only a getter
    at Object.<anonymous> (repl.js:17:20)
    at Module._compile (internal/modules/cjs/loader.js:759:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:770:10)
    at Module.load (internal/modules/cjs/loader.js:628:32)
    at Function.Module._load (internal/modules/cjs/loader.js:555:12)
    at Function.Module.runMain (internal/modules/cjs/loader.js:822:10)
    at internal/main/run_main_module.js:17:11
@lukastaegert
Copy link
Member

I see. So Rollup is basically interfering with its own logic here. I see at least two possible solutions:

  • change the reexport namespace copying logic to skip named exports; does not work well for multiple reexports, though
  • if a namespace is reexported, subsequent exports should not directly assign to the exports object but also use Object.defineProperty

@lukastaegert
Copy link
Member

Should also probably check how Babel handles this.

@klaussner
Copy link
Author

Babel creates an object with all named exports and skips the exports in the copying logic: Babel REPL link. But it looks like Babel doesn't handle overriding exports correctly, either. It works for named exports but if two export * statements are used, the code generated by Babel also fails with a TypeError if both modules have exports with the same name because the defined properties are not configurable and cannot be redefined: Babel REPL link (bug).

@lukastaegert
Copy link
Member

Thanks for checking. Not sure what to make of the second scenario, but at least the first is better than what we have at the moment.

As far as I know, the correct behaviour for conflicting namespace reexports is to remove the conflicting bindings. So if the bindings should be non-configurable, we would need to check and omit the bindings of the other export in such a scenario. I.e.

var _module = require("module-1");
var _module2 = require("module-2");

Object.keys(_module).forEach(function (key) {
  if (key === "default" || key === "__esModule" ||
      Object.prototype.hasOwnProperty.call(_module2, key)) return;
  Object.defineProperty(exports, key, {
    enumerable: true,
    get: function get() {
      return _module[key];
    }
  });
});

Object.keys(_module2).forEach(function (key) {
  if (key === "default" || key === "__esModule" ||
      Object.prototype.hasOwnProperty.call(_module, key)) return;
  Object.defineProperty(exports, key, {
    enumerable: true,
    get: function get() {
      return _module2[key];
    }
  });
});

Combining this with Babel's approach for the first scenario, I guess this should work, or am I overlooking something here?

@salmanm
Copy link

salmanm commented Oct 22, 2019

As a temporary workaround I've done the following and it works.

import * as fs from "fs";
const existsSync = ":)";

export default { ...fs, existsSync }

This way it doesn't try to do Object.defineProperty.

But would be good to see this one fixed.

@aleclarson
Copy link
Contributor

aleclarson commented Feb 11, 2021

I'm bumping into this. Is there a better workaround than using export default?

edit: Using output: { externalLiveBindings: false } solves the problem for me :)

@lukastaegert
Copy link
Member

Sorry for dropping the ball on this. Fix is here: #3959

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants