nodejs / node-eps

Node.js Enhancement Proposals for discussion on future API additions/changes to Node core
443 stars 66 forks source link

002 - Clarification on consuming CommonJS from ES modules #26

Closed nzakas closed 7 years ago

nzakas commented 8 years ago

In the ES modules proposal, there are several examples related to loading CommonJS modules from ES modules, but there is no example covering the use of this in an exported module. For example:

// CommonJS file util.js
module.exports.name = "commonjs";
module.exports.getName = function() {
    return this.name;
};

The getName() method makes use of this to determine the value to return. The proposal states that exports from CommonJS modules can be loaded in ES modules via import using those names:

import { getName } from "./util";

console.log(getName());              // what's the output?

My question is if the this binding remains in getName() or not? I would expect it to, but I don't see that mentioned in the proposal. In effect, that would mean the local binding getName in this example would not be directly equal to getName as exported from util.js but rather equivalent to getName.bind(module.exports).

Is that the way it's intended to work? If so, can that detail be added into the proposal?

jkrems commented 8 years ago

Why would it use this? Assuming that util.js isn't strict mode, it would return global.name. There's no this binding involved..? It becomes more tricky if the import would be:

import * as util from './util';

console.log(util.getName());
nzakas commented 8 years ago

Well, assume it's strict mode. The equivalent util.mjs would be:

export var name = "esmodule";

export function getName() {
    return name;
}

In which case, importing and calling just getName() would return the correct value.

I could be wrong, but even if I am, it seems like the correct behavior for this case should be explicitly called out.

bmeck commented 8 years ago

@nzakas We have no way to create a reference for the this binding to be correct. It will be the global this. We would need to do some spec work to enable something like a limited with if we wanted the correct value of this.

console.log(getName());              // global.name
bmeck commented 8 years ago

On a slightly different note, it is possible to do if the vms did variable expansion so

import {getName} from 'foo';
getName();

Approximately became:

import * as $foo from 'foo';
$foo.getName();

Though I don't see how this would be possible using ECMA262 currently.

nzakas commented 8 years ago

We have no way to create a reference for the this binding to be correct.

I'm not terribly familiar with Node.js internals, but it seems like when registering ES module bindings, you would be able to do module.getName.bind(module) or something similar? I'm assuming there's an abstract between the import statement and the call to require, in which you would be able to make whatever necessary changes might be required.

In any event, the high-level concern I had is that whatever the behavior will be, it's not very clear in the current proposal.

bmeck commented 8 years ago

@nzakas we could do that if we had clear timing on when exports change, we don't know that timing:

exports.name = 'exported';
exports.foo = function () {return this.name;};
// something mutates exports.foo (bluebird.promisfyAll, APM, spy, idk)

We would not know exactly when the export was changed, and it is unsafe for us to change all of the exports of a CJS module to a getter + the module.exports could be frozen.

nzakas commented 8 years ago

@bmeck hmm ok...I'm not sure I understand yet, but I'll dig through the spec another time rather than bothering you with more questions. Thanks for your time.

bmeck commented 8 years ago

@nzakas is it ok to close this issue, or do I need to clear up stuff still?

nzakas commented 8 years ago

I still think the proposal needs a precise description of what happens to the value of this inside of methods exported from CommonJS and imported into ES6.

bmeck commented 8 years ago

the this value is not bound, so normal access rules apply. by calling out that we will be delegating through an ObjectEnvironment however, the implied this of an import is the default export.

// a_promise.cjs
module.exports = Promise.resolve();

// consumer.es
import {then} from 'a_promise.cjs';
then(); // this value is the Promise, works
then.call(null); // this value is not a promise, Error from https://tc39.github.io/ecma262/#sec-promise.prototype.then

I guess I am missing something [about the question]. Nothing happens to the value of this inside of methods, but how the implicit this function is done via the ObjectEnvironment.

bmeck commented 8 years ago

will try and think of a way to phrase this. But, unsure how to phrase that simply, as no value for this is bound.

nzakas commented 8 years ago

Simply: is this equal to undefined or module.exports?

bmeck commented 8 years ago

@nzakas it is defined based upon access/invocation, it is not defined to an absolute value. Just like how this is not a well defined value just given a function declaration, I can't really answer that question.

In your specific example of:

import { getName } from "./util";

console.log(getName());  
nzakas commented 8 years ago

@bmeck so why can't you just say that? getName has this that is bound to module.exports?

bmeck commented 8 years ago

@nzakas because it is not bound like .bind, or the bind operator proposal:

console.log(getName.call({name:"not bound"}));

logs "not bound".

nzakas commented 8 years ago

Eh, I don't have enough energy to keep going around on this. My point remains: I believe the value of this is unclear in the proposal and should be clarified. If you disagree or if the proposal isn't going to be used at this point (for the unambiguous module thingy), then feel free to close.

bmeck commented 8 years ago

Something that might help clear things up would be an example of an ObjectEnvironment changing the this value without binding a function (courtesy of with):

const promprom = Promise.resolve('not bound');
function log(v) {
  console.log(v);
}
with (promprom) {
  then(log); // not bound
}
const then = promprom.then;
then(log); // #error

I understand it is not well stated, but it is defined in the proposal. I just have no clear way to express it in plain english in order to amend the proposal.

DanielRosenwasser commented 8 years ago

Note: This comment is also available at this repo.

Hey everyone, the TypeScript team has looked at various different facets of the module loading interop between CommonJS and ES. Our perspective is shaped by the following needs:

After discussing with @bterlson, @bmeck, @ajklein, @caridy, we feel that the following ideas address the concerns brought up above, while keeping these needs in mind.

No property plucking

The current proposal set forward states that a CommonJS module is primarily made available as a default import. The proposal then further has the notion of property plucking, where properties on the default import are also made available as named imports. This process is also called "hoisting".

This practice is likely to have certain negative consequences. One major issue is that it is not clear what host object a "plucked" named import is bound to (i.e. it isn't clear what the this value is). It is also not clear how get-accessors are treated under this system.

A more tangible issue for Node users is that this makes it difficult for library authors from migrate to ES modules, because naively doing so would cause breaks for ES consumers. For instance, consider the following file foo.js

module.exports = function() {
    // ...
};

module.exports.bar = "hello";

Imagine a consumer that is written using ES modules:

import f, { bar } from "./foo.js";

// 'f' is callable.
f();

// 'f' has a member named 'bar'.
f.bar.toLowerCase();

// We can also use 'bar' as a named import.
bar.toLowerCase();

Notice that because of property plucking from the default, bar was accessible as a named import as well.

Now when foo.js wants to migrate to ES module syntax, the author would likely write something like the following:

export default function() {
    // ...
};

export var bar = "hello";

However, breaks the usage of f.bar.toLowerCase() in the above example! Another naive fix might have been the following:

function d() {
    // ...
};
d.bar = "hello";

export default d;

However, this breaks the usage of bar as a named import! Instead, the library author must re-export each member of their default export to maintain compatibility with ES consumers. This is a strong disincentive for moving to ES modules.

We believe that by default, a CommonJS module should only be made available using a default import.

Transpilation Support

Interop can work well enough natively between ES modules and any existing module system if it plans for it. That is, we can plan out the interop behavior between CommonJS and ES modules for the future, but that means that there is still a gap for people on older versions of Node. Transpilers like TypeScript and Babel fill in that gap so that users can still author in ES but target older versions of Node.

If CommonJS modules are only brought in as a default, it becomes impossible to define named exports for ES consumers. Modules also need to be able to affect the shape of the namespace import.

One way to enable this is to "pluck" properties as above, however:

The __esModule property is something that both Babel and TypeScript emit in some capacity today, and is also recognized in SystemJS. The necessity was recognized by Guy Bedford in 2013 in working on es6-module-transpiler (@guybedford). Basically what it boils down to is that CommonJS modules need to be able to dictate whether their shape should describe a default import or the namespace import.

In the case that an __esModule property is present on the module.exports object, it should act as a signal to the loader that the value of module.exports describes the namespace import.

For instance:

// CJS library a.js
module.exports.greeting = "hello!";

// CJS library b.js
module.exports.farewell = "hello!";
module.exports.__esModule

//
// ES consumer:
//
import a from "./a.js";
import * as b from "./b.js";

// 'greeting' is accessible on the default import.
a.greeting.toLowerCase();

// 'farewell' is accessible on the namespace import.
b.farewell.toUpperCase();

Default Substitution for require

We mentioned above that authors are likely to convert module.exports = ... to export default .... Problematically, this means CommonJS consumers are immediately broken if default exports are only accessible through require(...).defualt.

As a fix to avoid breaking CommonJS consumers, require should adopt two steps prior to returning:

  1. If the require'd module is an ES module, and a property named __esModule is not exposed on the result, a non-enumerable property of that name is added and set to true.
  2. If __esModule is present and the only other property on the result is named default, then the value of default is returned instead.

This means that for the following ES module

export default function(a, b, c) {
    // ...
};

you may import it as follows:

var foo = require("./foo");
foo(1, 2, 3);

This makes compatibility easy for users on the current runtime. It also makes it possible to patch up the behavior of old runtimes to allow transpiled modules to work the same.

Summary

  1. If an ES Module is require(...)'d, then it automatically gets a non-enumerable property named __esModule.
  2. If a CommonJS module is imported by an ES module, then
    • If the CJS module has a __esModule property on its module.exports object, that module.exports object is used in place of the namespace export.
    • Otherwise, a CJS module is only made available as a default import. No named properties are made available.
  3. If the result of a call to require(...) has only a property named default as well as a property named __esModule, then default is used in place of the original result.
unional commented 8 years ago

Also please remember the transitivity between CommonJS <-> ES modules.

bmeck commented 7 years ago

Status update on transitivity and hoisting/this/etc:

After several talks: VMs cannot implement property hoisting, it invalidates a lot of assumptions about what can happen on variable access and does not fit in current codegen models. It could theoretically be possible to have hoisting, but still unsure / has other issues.

The main other issue is transitivity. Given:

// a
export let foo = 0;
setInterval(() => { foo+= 1; }, 1000)
// b
module.exports = require('a');
// c
import {foo} from 'b';

c should have its import at exactly the same as import {foo} from 'a';.

In the case of hoisting, this works, but has the default caveat like listed above.

Without hoisting, any import will need to have knowledge that something is really an ES module namespace. Like the __esModule flag also listed above.

Whatever the eventual solution, the browser will need to be compatible with Reflective Module Records.

I am unclear that __esModule as it stands will work with the browser spec, since we don't have a synchronous Object.observe basically. The WHATWG spec uses a mutator, that needs to have .set(exportIdentifier, value) called manually whenever an export needs to be changed. I am still thinking about things. If transpilers had hooks for watching when exported variables change things could work, but that is somewhat different from how things work today.

Still thinking about things though, just giving a small update / summary of current situation.

guybedford commented 7 years ago

Is there a reason we can't just assume that named exports from CommonJS are just treated as snapshots and not live bindings? Then it would be possible to do function binding as well etc. Yes it's limited, but doesn't that capture the majority use case where users need named exports, without heavy cost?

Then if users really need live bindings, import the default export, and access the member property rather?

Also my two cents on esModule - it was very much to deal with transpilation interop. When real ES modules objects are run against this transpiled code, the plan was to extend the interop to check not just `esModulebut also am.toString() == '[object Module]'` to ensure the transpiler interop works out correctly in true ES module environments.

bmeck commented 7 years ago

As mentioned in last CTC named imports from CJS are not going to land in the initial implementation due to inability to have property delegation and synchronization issues.