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

Destructing CommonJS export with import #1483

Open
OliverJAsh opened this issue Nov 6, 2014 · 18 comments
Open

Destructing CommonJS export with import #1483

OliverJAsh opened this issue Nov 6, 2014 · 18 comments

Comments

@OliverJAsh
Copy link
Contributor

// foo.js (CommonJS)
module.exports = { bar: 'baz.js' }
// (ES6 module)
import { bar } from 'foo.js';
console.log(bar); // => undefined

How come this doesn't work?

@guybedford
Copy link
Contributor

We're discussing making this work at the moment actually in babel/babel#95.

@UltCombo
Copy link
Contributor

UltCombo commented Nov 6, 2014

In which environment are you running this? Are you compiling to commonjs/AMD or using traceur.require.makeDefault()?

@OliverJAsh
Copy link
Contributor Author

@UltCombo I'm using traceur.require.makeDefault.

@UltCombo
Copy link
Contributor

UltCombo commented Nov 6, 2014

Trying to compile your ES6 sample to CJS also fails with the error 'bar' is not exported by 'path/to/foo.js'. @guybedford has been working on the CJS interop for a good while now, so he most likely knows why this isn't working.

@domenic
Copy link
Contributor

domenic commented Nov 6, 2014

I do not think this makes sense. Remember that import { x } from "y" is not destructuring; it is importing named exports. CommonJS modules in general only have default exports (the module.exports value, which is equal to the exports object if unset). So you should never be able to import named exports from them.

@UltCombo
Copy link
Contributor

UltCombo commented Nov 6, 2014

CommonJS modules in general only have default exports

Oh yes, @domenic is right, the CJS module.exports -> ES6 default export translation logic was implemented by @guybedford in #818. The reasoning is explained in these comments:

@guybedford
Copy link
Contributor

Well this is an important thing to discuss!

We can do it, by making the module for a CommonJS module effectively:

function createES6Module(cjsModule) {
  var es6Module = {};
  // exports copied from CommonJS object
  Object.keys(cjsModule).forEach(function(export) {
    es6Module[export] = cjsModule[export];
  });
  // default export is entire CommonJS object
  es6Module.default = cjsModule;
}

So the question is whether we want this decorated CommonJS ES6 form supported or not (to allow import {readFile} from 'fs'). It can work out, so the question is entirely about best-practice.

@UltCombo
Copy link
Contributor

UltCombo commented Nov 6, 2014

@guybedford as a side effect, wouldn't that allow both import * as x from 'x' and import x from 'x' to have access to the CJS "named" exports?

@domenic
Copy link
Contributor

domenic commented Nov 6, 2014

IMO we should not go to that trouble. (Because then you have to worry about when import x from "y" transpiles to var x = require("y") versus var x = require("y").default, and then you start advocating for __esModule and Reflect.isModule and other things which IMO are way too complicated.)

IMO our story should be:

  • The simplest solution is to use require for CommonJS modules and import for ES6 modules. This is for example what I do in my projects.
  • If you are a person who hates seeing require in their code for some reason, you can instead use import x from "y" and if "y" resolves to a CommonJS module, it will treat the CommonJS module as an ES6 module with only a default export.
  • If you want to use import { x } from "y", then "y" has to be an ES6 module, since only ES6 modules have the concept of named exports.

In this way the interop story is a simple band-aid for people who hate to see the word require, but it doesn't obscure the underlying model of what is actually going on.

@guybedford
Copy link
Contributor

Yes I've advocated for the simple interop as you've mentioned above, but was under the impression others were keen to see exports for CommonJS imports (@caridy).

Note that the __esModule and Reflect.isModule are required regardless of this decision though.

@guybedford
Copy link
Contributor

It's not about being a person who hates seeing require in ES6 - it is about the fact that loading CommonJS with ES6 syntax is a fundamental interop use case of the module loader.

@caridy
Copy link

caridy commented Nov 6, 2014

@guybedford I'm supportive of @domenic suggestions, I think that is good enough to facilitate the transition, and I have been pushing back on the __esModule trick for a while.

On the flip side of that coin, to require a ES6 module from a CommonJS module, we are advocating for a very simple shim, created manually by the pkg author, to simply define the module.exports with a manual mapping of the exported structure. E.g.: https://github.com/yahoo/intl-messageformat/blob/master/index.js

@guybedford
Copy link
Contributor

@caridy @domenic excellent - do we agree then that import {readFile} from 'fs' should not be supported when loading CommonJS from ES6, and we should always use the default import?

@caridy
Copy link

caridy commented Nov 6, 2014

correct, e.g.:

import fs from "fs";
export {default as fsObj} from "fs";
export var readFile = fs.readFile;

@UltCombo
Copy link
Contributor

UltCombo commented Nov 6, 2014

@guybedford From my point of view, transpilers should follow spec'd behavior. AFAIK, Node.js has no solid plans to support ES6 modules yet. It looks like you're trying to p(r)olyfill ES6 modules support into Node, but instead your logic simply transpiles ES6 syntax into CJS syntax. IMO this is bad because:

  • The user code (ES6) is implicitly using mechanics that are proprietary to Node.js (CJS require and module path resolution algorithm).
  • Due to the point above, one can't cross-load CJS in any environment other than Node.js and your "interop" is actually just sugaring on top of proprietary mechanisms.
  • It has already been said that __esModule makes users' ES6 code dependent on the transpiler. An ES6 transpiler should be just a temporary step, not an obligatory dependency.
  • (As far as I can see) The best way to achieve cross-loading CJS modules using ES6 modules syntax is to effectively rewrite the CJS and Node's module path resolution logic on top of the ES6 Modules' semantics, using the normalize hooks and whatnot. But this is a lot of work.

@caridy
Copy link

caridy commented Nov 6, 2014

@UltCombo agreed, but this is not different from using cjs modules in a browser (thru browserify or webpack), where you need a build process to produce code for a different runtime. In most cases, we end up authoring modules in one format, and compiling it into distros for the different runtimes, and supporting interoperability thru all those different formats.

In our case, the whole FormatJS project is written in ES6, each library is transpiled into two other formats: bundle format (browser distro) and cjs (nodejs distro), but still you can create a new library that depends on some of those libraries using the ES6 semantics by depending on the ES6 source code directly by using the esnext:main directive in package.json.

It works very well, but, yes, it is a lot of extra work and boilerplate to achieve it. And obviously, this is only needed for a library, apps are a completely different story.

@caridy
Copy link

caridy commented Nov 6, 2014

... and one more note, when we transpile, the intention is always to output a module that works with the semantics of the output format. Doing it the other way around to try to bring alien semantics to one of those runtimes will be a mistake.

@UltCombo
Copy link
Contributor

UltCombo commented Nov 6, 2014

@caridy thanks for the insight, it makes a lot of sense.

I wonder if it would at all be possible to simplify the overall process. My initial thought was to use System.translate to transform require() expressions into import statements, but that would be a lot of effort and would still run into the issues highlighted above, and have the same limitations as browserify.

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

5 participants