Skip to content
This repository has been archived by the owner on Mar 29, 2018. It is now read-only.

__esModule flag #86

Open
guybedford opened this issue Dec 26, 2013 · 14 comments
Open

__esModule flag #86

guybedford opened this issue Dec 26, 2013 · 14 comments
Labels

Comments

@guybedford
Copy link
Contributor

Update - calling this __esModule.

Working with module loaders based on https://github.com/ModuleLoader/es6-module-loader, in order to support the loading of AMD and CommonJS, it is necessary to add a flag to transpiled modules.

This flag is completely necessary to support AMD, ES6 and transpiled ES6 for module loaders. I have attempted to explain why below, but if it is unclear please do ask.

If we can agree on the name of a __transpiled flag, then we save much headache further down the line.

To explain how this occurs, consider loading an AMD module from an ES6 module loader:

  define(function() {
    return function() {
      console.log('exported value');
    }
  });

In the module table, we need to store the entire AMD module as the default value:

new Module({
  default: function() {
    console.log('exported value');
  }
});

Now when loading this AMD module from within another AMD module, we then need to automatically provide the default property to the module.

This is all great, but now if we load an ES6 module transpiled as AMD, from with an AMD module, we will try to do the same trick, and end up providing the default property of a transpiled es6 module instead of the entire es6 module to the AMD module.

A way to avoid this is to simply define a flag on the transpiled module so we know not to do this:

  // transpiled from ES6:
  define({
    __transpiled: true,
    someExport: 'value'
  });

This then allows AMD modules the full access to other AMD modules, ES6 modules as well as transpiled ES6 modules all in consistent ways.

@guybedford
Copy link
Contributor Author

An alternative name for this __transpile flag is __module.

Consider that I could write a CommonJS module like:

  exports.someExport = 'hello world';
  exports.__module = true;

Which would then correspond directly to the ES6 module object (indicated by the __module flag):

  new Module({
    someExport: 'hello world'
  });

Instead of the standard default.

For this use case flexiblity, I'm tending towards liking the name __module over __transpiled.

@joliss
Copy link

joliss commented Dec 30, 2013

I've responded to this on #85 (comment). Let's continue the discussion there?

@guybedford
Copy link
Contributor Author

I think this has finally converged with #85.

We're now calling this __esModule inline with #85 in Traceur.

So the output is to add the __esModule: true export when compiling to CommonJS and AMD.

Basically this is necessary to allow AMD, CommonJS and ES6 to work together in ES6 and other loaders. I would strongly advise getting this in sooner rather than later. Without it, transpiled ES6 will not interop with transpiled ES6 that loads AMD or CommonJS.

I didn't want to spell out more technical details right here, but @thomasboyt if you are interested please chat to me anytime and I'll explain further.

@guybedford
Copy link
Contributor Author

@thomasboyt just checking if this is something that may still be considered? Just had a user of SystemJS unable to use CommonJS transpiled modules in SystemJS and a flag like this is crucial to that use case. Let me know if I can help.

@guybedford
Copy link
Contributor Author

I just want to bump this again, because I do think it's important to ensure transpiled ES6 modules in the wild behave correctly in an ES6 loader. If I submit a pull request would it be considered?

@thomasboyt
Copy link
Contributor

Sorry, this is definitely something that we're doing, I should have commented on this way earlier.

It's in the rewrite branch here: https://github.com/square/es6-module-transpiler/blob/recast/src/rewriters/cjs_rewriter.js#L10 (though it looks like I'll need to update it to __esModule)

Unfortunately I haven't managed to finish that rewrite. If you'd like to do a PR on the current version, I'd be happy to review+merge+release this feature.

@guybedford
Copy link
Contributor Author

Amazing, thank you. Are you happy to work with the __esModule name? I had a look at the code for this version, but couldn't tell how I would go about adding this flag unfortunately. Advice welcome if you have any ideas.

@eventualbuddha
Copy link
Contributor

So this is still unimplemented as of v0.5.0. If anyone cares to take a stab at altering the CJS formatter to do this, I'd look at a PR.

@guybedford
Copy link
Contributor Author

After all the discussion, I'm actually wondering if we should move away from this perhaps.

The reason this flag was added was because when I transpile ES6 into AMD or CommonJS, it then will behave as AMD or CommonJS, instead of behaving as ES6.

So if I put that module into the ES6 Module Loader, it wouldn't be able to work like an ES6 module, loading named dependencies from other ES6 modules.

All of this was because we were originally looking at AMD as being the format to convert ES6 into, but we have have since changed this format to being System.register.

So I think perhaps we should leave this out, until such a point as we see a real need, in which case we can reconsider.

Really appreciated for the follow up, but will close for now.

@guybedford
Copy link
Contributor Author

This issue came up again recently on es-discuss, where there was a scenario compiling ES6 into CommonJS, and then loading normal CommonJS modules using module syntax.

They were writing:

  module fs from 'fs'

to get the output:

  var fs = require('fs');

When this should really be:

  import fs from 'fs';

A flag and a conditional check are still necessary for this interop scenario. So perhaps we should reopen this.

@guybedford guybedford reopened this Jul 24, 2014
@caridy
Copy link
Contributor

caridy commented Oct 13, 2014

@guybedford can we close this now?

@guybedford
Copy link
Contributor Author

I believe it is still necessary for interop, it would be good to discuss.

@wycats
Copy link
Contributor

wycats commented Dec 6, 2014

I feel that @caridy and @ericf's solution to generate an index.js automatically with manual override for curation solves the underlying problem. Do you disagree?

@guybedford
Copy link
Contributor Author

@wycats I agree that is a great solution, but these problems have many angles, and we can't dictate that workflow entirely.

The issue here specifically is that when we allow users to separately compile ES6 into CommonJS files, they will (and I have seen this in Traceur) load CommonJS from that compiled ES6 CommonJS.

With naive transformation, this means users end up with code like:

import * as fs from 'fs';
import {readFile} from 'fs';

which works in the output, but is not what we want people to be writing, but is their only option since import fs from 'fs' doesn't work.

So handling the interop upfront will avoid users writing invalid ES6 and make sure that they write ES6 that loads CommonJS in the right way.

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

No branches or pull requests

6 participants