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

Call out singleton module concerns around dual-mode packages #10

Closed
jkrems opened this issue Nov 7, 2018 · 12 comments
Closed

Call out singleton module concerns around dual-mode packages #10

jkrems opened this issue Nov 7, 2018 · 12 comments

Comments

@jkrems
Copy link
Owner

jkrems commented Nov 7, 2018

The solution to allow two independent entrypoints for CJS and ESM introduces one fundamental issue which is that there are now potentially two copies of the module. Example:

{
  "name": "render-lib",
  "main": "./cjs.js",
  "exports": "./esm.mjs",
}
esm.mjs
export class Element {}

export function render(element) {
  if (!(element instanceof Element)) throw new TypeError('needs element');
}
// cjs.js
// Assumption: cjs.js is generated from esm.mjs via a compiler like babel
exports.Element = class Element {};

exports.render = function render(element) {
  if (!(element instanceof exports.Element)) throw new TypeError('needs element');
};

A user of this library might attempt the following:

// red-element.js
const { Element } = require('render-lib');
module.exports = class RedElement extends Element {};
// main.js
import { render } from 'render-lib';
// Short-cut, might be using createRequireFunction etc.
import RedElement from './red-element.js';

// This throws because RedElement inherited a *different* Element class,
// not the one defined in the file we imported above.
render(new RedElement());

This problem appeared in the wild in GraphQL.

/cc @bmeck who reported this issue.

@bmeck
Copy link

bmeck commented Nov 7, 2018

You can search around github to see this problem of having 2 modules trying to act as if they are a single module in various places like the create-react-app issue tracker as well.

Not only is instanceof affected but also any sort of module scoped variables like:

let id = 0;
export function foo() {
  return {id: id++};
}

and

let id = 0;
module.exports = {
  foo: function foo() {
    return {id: id++};
  }
}

@jkrems
Copy link
Owner Author

jkrems commented Nov 7, 2018

Yes, instanceof was just one. The issue title tries to capture the general concern by calling it "singleton module concerns". E.g. any concerns connected to there suddenly being two instances of the same package.

@GeoffreyBooth
Copy link
Collaborator

When would the same project import both the ESM and the CommonJS versions of the same module? If I’m writing a project in ESM, wouldn’t all my import statements import the ESM version? I suppose if I use createRequire then I’ll get the other, but then I’m going out of my way to import the CommonJS version.

I guess is the use case an “in migration” project where some of my files are ESM and some are still CommonJS, and the CommonJS files require the CommonJS version of the package? So the red-element.js file in the first example is CommonJS and main.js is ESM, and they import the CommonJS and ESM versions respectively? The above example doesn’t work in our existing implementation, though I suppose it could if main.js imported red-element.js via createRequire.

@bmeck
Copy link

bmeck commented Nov 8, 2018

@GeoffreyBooth but other 3rd party packages could be using require() still. Not all module loading is done in a single project generally.

@GeoffreyBooth
Copy link
Collaborator

I’m not sure this really is resolvable. Say I create a package like this:

package.json

{
  "name": "sneaky-package",
  "main": "./cjs.js",
  "exports": "./esm.mjs"
}

cjs.js

module.exports = function () {
  console.log('Arrr! X marks the spot!');
}

esm.mjs

export default function () {
  console.log('Arrr! O marks the spot!');
}

These really are two distinct packages. Even if we looked at the module map to see that require('sneaky-package') and import 'sneaky-package' reference the same package, the code exported by the package is different between ESM and CommonJS, and will always need to be as long as we allow dual-mode packages.

@jkrems
Copy link
Owner Author

jkrems commented Nov 8, 2018

At its core, I do agree with what @GeoffreyBooth is saying. I don't think this is something that necessarily needs a technical solution. But we can (try to) be helpful. E.g. we could write a deprecation warning when the same module is loaded in both ways. Or we could publish clear guidance for package maintainers that warns of these issues and points out design patterns.

@bmeck
Copy link

bmeck commented Nov 8, 2018

I'm fine with either, I'd also be fine with throwing if both fields exist.

@GeoffreyBooth
Copy link
Collaborator

I don’t think we need to throw purely if both fields exist; that would effectively prohibit dual-mode packages. Instead we could throw if both versions are loaded.

The question is, do we need to throw if both versions are loaded anywhere in the module graph, or only if both are loaded into the same package? Like if my app imports lodash (esm) and request (cjs), and request requires lodash (cjs), is that an issue or does the package boundary protect me from any conflict?

@bmeck
Copy link

bmeck commented Nov 8, 2018

@GeoffreyBooth that boundary does not necessarily protect you.

@jkrems
Copy link
Owner Author

jkrems commented Nov 8, 2018

@GeoffreyBooth Pretend red-element is itself an npm package (e.g. a reusable react component). The package boundary doesn't really matter for this issue unfortunately.

@GeoffreyBooth
Copy link
Collaborator

Okay, so is my conclusion correct then? We only throw if both the CommonJS and ESM versions of a dual-mode package are loaded?

@bmeck would it be feasible to update Node to take advantage of package boundaries to gain “protection,” or is that just not possible because of I’m sure various very good reasons?

How does Node deal with two different versions of the same module being loaded by an app? For example, my app directly imports request and mime-types@2.1.21, and then request itself imports mime-types@2.1.19—I assume both my app and request each get the (separate) version of mime-types that they want?

@jkrems
Copy link
Owner Author

jkrems commented Aug 15, 2019

Since exports is now used in CJS as well, this shouldn't apply anymore. At most we would shadow main in both ESM and CJS iff we bring back dot-main.

@jkrems jkrems closed this as completed Aug 15, 2019
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

No branches or pull requests

3 participants