Skip to content
This repository has been archived by the owner on Jun 4, 2024. It is now read-only.

CJS interop? #12

Open
bmeck opened this issue Dec 7, 2016 · 19 comments
Open

CJS interop? #12

bmeck opened this issue Dec 7, 2016 · 19 comments

Comments

@bmeck
Copy link

bmeck commented Dec 7, 2016

To my understanding this will load CJS files and have named property access on them? There is ongoing discussion if this is going to be implemented or not. I am hesitant to encourage named properties working. In all proposed interopoperability scenarios however, there is a default export that maps directly to module.exports. Something to bear in mind, and perhaps remove named imports until interop is ironed out.

@bmeck
Copy link
Author

bmeck commented Dec 7, 2016

I bring this up because, whew boy are we in enough of the weeds due to incompatibilities of existing CJS interop w/ babel and ESM

@bmeck
Copy link
Author

bmeck commented Dec 7, 2016

Example requested by https://twitter.com/dan_abramov/status/806506649544491008

This plugin maps import() to require.ensure()

import returns the ModuleNamespaceObject of an ESM.

require.ensure returns the value of module.exports directly.

CJS interop with ESM cannot create a ModuleNamespaceObject that can proxy to all possible shapes of module.exports.

Due to this all proposals for interop starting with the original EP have used wrapping mechanics to produce ModuleNamespaceObjects. The exact behavior of wrapping mechanics is still in discussion from talks with VM implementors and TC39 started in September. However, all proposals require the wrapping mechanics to provide a default and vary on the behavior of named exports.

A good example of breakage should people expect exact mapping to require.ensure:

(await import('fs')).readFile; // may not work depending on wrapping mechanics

However, if we use the default that exists in all proposals:

(await import('fs')).default.readFile; // will work in all proposals

To note: this is a babel plugin and should detect non-CJS using __esModule on CJS to determine if it is acting as an ESM. If it is acting as an ESM it should be sane to not wrap.

@bmeck
Copy link
Author

bmeck commented Dec 7, 2016

ah, conversely in the current code:

(await import('fs')).default.readFile;

Does not work, since it doesn't shadow nor wrap default

@Jessidhia
Copy link

Jessidhia commented Dec 7, 2016

Sounds like migrating to that is going to be particularly painful for TypeScript... it maps commonjs module.exports directly to the namespace, and requires that you do fun things such as call a namespace import as a function if the commonjs exports was a function, etc...

I think that solution will be a good thing (you can't statically verify the imported symbols otherwise), but sounds like it'll take time 😅

@bmeck
Copy link
Author

bmeck commented Dec 7, 2016

@Kovensky I've had talks w/ typescript in the past about this, they are aware. This behavior also affects import declarations.

@Jessidhia
Copy link

Jessidhia commented Dec 7, 2016

I had actually expected implementers of a dynamic import plugin for webpack to actually map to System.import, which on webpack's implementation should already have the wrap-in-default behavior for non-ESM (...I think). I presume this went with require.ensure for webpack 1 compatibility...

@TheLarkInn
Copy link
Contributor

Yes the import() we have implemented is more similar to System.import

@aluanhaddad
Copy link

@Kovensky just in case you were unaware, TypeScript has a flag to model the synthesis of a default export from a CommonJS module. --allowSyntheticDefaultImports

@ljharb
Copy link
Collaborator

ljharb commented Dec 7, 2016

@bmeck at the moment I'm operating under the assumption/belief/hope that it will indeed allow named property access. Once the interop story gets more concrete, we can ship a breaking change to this transform that implements the proper semantics.

@bmeck
Copy link
Author

bmeck commented Dec 7, 2016

@ljharb can we at least properly set the default?

@ljharb
Copy link
Collaborator

ljharb commented Dec 7, 2016

With the way it's implemented right now, won't Babel do the correctish thing wrt default and named exports?

@bmeck
Copy link
Author

bmeck commented Dec 7, 2016

@ljharb no, you need something like:

  (new Promise((resolve) => {
    require.ensure([], (require) => {
      var ns = require(SOURCE);
      if (ns && ns.__esModule) {
        resolve(ns);
      } else {
        var wrapper = Object.create(ns);
        wrapper.__esModule = true;
        wrapper.default = ns;
        Object.freeze(wrapper);
        resolve(wrapper);
      }
    });
  }))

to get close-ish

@ljharb
Copy link
Collaborator

ljharb commented Dec 7, 2016

Right, but if the module.exports being import()ed lacks __esModule, then Babel turns named imports into property access - and if has it, then Babel will pull .default - no?

@bmeck
Copy link
Author

bmeck commented Dec 7, 2016

@ljharb existing shim in babel below:

function _interopRequireWildcard(obj) {
  if (obj && obj.__esModule) { return obj; }
  else {
    var newObj = {};
    if (obj != null) {
      for (var key in obj) {
        if (Object.prototype.hasOwnProperty.call(obj, key)) newObj[key] = obj[key];
      }
    }
   newObj.default = obj; return newObj;
  }
}
  • .default is shadowed, but re-export problem from not setting __esModule
  • prototype is incorrect
  • named imports don't reflect inherited properties (this is one thing under discussion)
  • getter don't fully function, mocking/apm/promisifyAll breakage (this is one thing under discussion)
  • namespace is mutable

@Jessidhia
Copy link

Jessidhia commented Dec 8, 2016

@bmeck How does something like this sound?

function _specInteropRequireWildcard(obj) {
  if (obj && obj.__esModule) {
    return obj;
  } else {
    var newObj = Object.create
      ? Object.create(null, {
        default: {
          value: obj,
          writable: true,
          enumerable: true
        },
        __esModule: {
          value: true
        }
      })
      : {
        default: obj,
        __esModule: true
      };
    if (typeof Symbol !== "undefined" && Symbol.toStringTag) {
      Object.defineProperty(newObj, Symbol.toStringTag, { value: "Module" });
    }
    return (Object.freeze || Object)(newObj);
  }
}

I've been discussing with @loganfsmyth on slack about implementing a possible spec: true version of the transform that could use that improved helper, as well as more spec compliant exports (live binding, immutable module record).

@bmeck
Copy link
Author

bmeck commented Dec 8, 2016

@Kovensky sounds good. eventually would be interested in test262 being brought into all this to see % compliance for babel:

TEST_FILES="$(find test262/test -name \*.js)"
mkdir compiled-test262
for FILE in $TEST_FILES; do
  babel -o compiled-$FILE < $FILE
done
// with .babelrc for transform-es2015-modules-commonjs
for FILE in $TEST_FILES; do
  node --require add-assert-global.js compiled-$FILE 
done

but not really going to push that right now since it just checks the runtime behavior of ModuleNamespace, not the actual interop properties.

@Jessidhia
Copy link

@aluanhaddad does enabling --allowSyntheticDefaultImports also emit appropriate wrapping code, or does it need to be used in combination with both --modules=es6 and babel/bundler postprocessing?

@bmeck
Copy link
Author

bmeck commented Dec 13, 2016

@Kovensky<