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

RFC - Dealing with imports/exports #23

Closed
bebraw opened this issue Mar 7, 2017 · 20 comments
Closed

RFC - Dealing with imports/exports #23

bebraw opened this issue Mar 7, 2017 · 20 comments

Comments

@bebraw
Copy link
Contributor

bebraw commented Mar 7, 2017

Context: webpack-contrib/file-loader#130 (comment)

If webpack packages use import/export semantics and don't transpile, we'll end up with issues like this:

file-loader [feat-webpack-defaults] $ npm run build

> [email protected] prebuild .../file-loader
> yarn run clean:dist

yarn run v0.20.3
$ del-cli dist
✨  Done in 0.50s.

> [email protected] build .../file-loader
> cross-env NODE_ENV=production babel -s true src -d dist --ignore 'src/**/*.test.js'

src/index.js -> dist/index.js
file-loader [feat-webpack-defaults] $ node
> require('./')
.../file-loader/dist/index.js:5
import loaderUtils from 'loader-utils';
^^^^^^
SyntaxError: Unexpected token import

The problem is that if we do export default something, then that will lead to the .default issue with CommonJS environments.

Any ideas on a clean way to handle this? Maybe packages should use ES6 style internally and expose their public API through a CommonJS module.exports?

@joshwiens
Copy link
Member

I'll tweak the babel config

@joshwiens
Copy link
Member

The only viable answer is to transpile it down into ES5 and pray to the NodeJS gods that they sort out the ES Module thing finally so we can stop using require( )

@bebraw
Copy link
Contributor Author

bebraw commented Mar 7, 2017

Yeah. I saw a blog post month ago where the ETA was a year. I think the plan is to end up with .mjs (ES6) and transpile that down to .js. But it's theoretical.

I think the only thing that remains is figuring out how to deal with export default. One way to do it would be to write a small CommonJS adapter to packages that uses their semantics and adapts to Node.

@joshwiens
Copy link
Member

joshwiens commented Mar 8, 2017

Sticking this here for reference output of the current Babel configuration ..

file-loader transpiled output using webpack-defaults#master

'use strict';

Object.defineProperty(exports, "__esModule", {
  value: true
});
exports.raw = undefined;
exports.default = fileLoader;

var _loaderUtils = require('loader-utils');

var _loaderUtils2 = _interopRequireDefault(_loaderUtils);

function _interopRequireDefault(obj) { return obj && obj.__esModule ? obj : { default: obj }; }

function fileLoader(content) {
  this.cacheable && this.cacheable(); // eslint-disable-line no-unused-expressions

  if (!this.emitFile) throw new Error('emitFile is required from module system');

  var query = _loaderUtils2.default.getOptions(this) || {};
  var configKey = query.config || 'fileLoader';
  var options = this.options[configKey] || {};

  var config = {
    publicPath: false,
    name: '[hash].[ext]'
  };

  // options takes precedence over config
  Object.keys(options).forEach(function (attr) {
    config[attr] = options[attr];
  });

  // query takes precedence over config and options
  Object.keys(query).forEach(function (attr) {
    config[attr] = query[attr];
  });

  var url = _loaderUtils2.default.interpolateName(this, config.name, {
    context: config.context || this.options.context,
    content,
    regExp: config.regExp
  });

  var outputPath = url;

  var publicPath = `__webpack_public_path__ + ${JSON.stringify(url)}`;

  if (config.outputPath) {
    // support functions as outputPath to generate them dynamically
    outputPath = typeof config.outputPath === 'function' ? config.outputPath(url) : config.outputPath + url;
  }

  if (config.publicPath) {
    // support functions as publicPath to generate them dynamically
    publicPath = JSON.stringify(typeof config.publicPath === 'function' ? config.publicPath(url) : config.publicPath + url);
  }

  if (query.emitFile === undefined || query.emitFile) {
    // eslint-disable-line no-undefined
    this.emitFile(outputPath, content);
  }

  return `export default = ${publicPath};`;
} /*
    MIT License http://www.opensource.org/licenses/mit-license.php
    Author Tobias Koppers @sokra
  */
var raw = exports.raw = true;
//# sourceMappingURL=index.js.map

@bebraw
Copy link
Contributor Author

bebraw commented Mar 8, 2017

My only worry is CommonJS/ES6 interop. I mean this (repl against file-loader webpack-defaults PR):

file-loader [feat-webpack-defaults] $ node
> require('./')
{ raw: true, default: [Function: fileLoader] }

It would be good to ask Henry or someone in the know to figure out the right way to handle this. export default bites pretty hard on CommonJS side of the fence.

@joshwiens
Copy link
Member

We can just as easily use a named export though that really doesn't make any sense.

And interop with what exactly? From what I can read, Webpack handles export default just fine.
Given this being supported natively in node is coming, shouldn't we be looking at where the interop problems are and fixing this on the end that is actually the problem?

@bebraw
Copy link
Contributor Author

bebraw commented Mar 8, 2017

We can just as easily use a named export though that really doesn't make any sense.

Yeah, named export wouldn't work either.

And interop with what exactly? From what I can read, Webpack handles export default just fine.
Given this being supported natively in node is coming, shouldn't we be looking at where the interop problems are and fixing this on the end that is actually the problem?

Webpack side isn't a problem as it's easy to manage there. If we go with defaults, that's a breaking change for those that consume functionality standalone for reason or another. But that can be handled through a major version bump and if you consume loaders/plugins outside of webpack, you can always do require('foobar').default.

@joshwiens
Copy link
Member

So rule number 1, an upgrade to defaults is a semver MAJOR without exception.

To that end, we should not release any majors in contrib so no loaders / plugins get them back to back which just screams poor planning.

Rule #2, every major gets at least on x.x.x-beta.0 release with said major as the npm beta dist-tag.

require('foobar').default seems perfectly acceptable and should be documented in the release notes though I would also like to get @hzoo's take on all of this given he probably has a deeper understanding of the issue than the two of us combined.

@bebraw
Copy link
Contributor Author

bebraw commented Mar 8, 2017

Major bump + clear communication sounds like the way. The worst thing that could happen is that someone on webpack 1 installs a package without knowing about the change. It's an instant error unless the an adapter against .default is ported to webpack 1 logic (might be a good move).

Beta sounds fair too.

@joshwiens
Copy link
Member

I sent Henry a message on Slack, before we start propagating this to everything in the Org he has recommended asking @loganfsmyth & @kovensky for their advice.

@loganfsmyth
Copy link

Is there a quick summary of what you're changing? I don't think I have enough enough context in the conversation here.

@bebraw
Copy link
Contributor Author

bebraw commented Mar 8, 2017

@loganfsmyth It's about loader/plugin packaging. Currently most, if not all, use CommonJS (no preprocessing). With the advent of webpack-defaults we are changing things around. Now distribution code will be processed through Babel.

The question is, what's the standard way of dealing with exports in this case assuming you are transpiling from ES6 module definition?

@loganfsmyth
Copy link

You'd be distributing CommonJS modules that have been output by the ES6 module compilation step in Babel, got it.

I think the most important thing to remember is that the interop story between CommonJS and real ES6 modules is going to be up in the air for a while, as you mentioned above. We can make guesses about it, but at the end of the day the CommonJS interface is king, with Babel's interop if you happen to use Babel's compiled import being a nice bonus on top. This essentially means if you use Babel's output as the public interface for a module on NPM, you have take into account both the standard CommonJS usage, and the usage alongside Babel's import.

As you've said, Babel's standard approach is to define a property for each export on the exports object, so when used via CommonJS, people will need to require('foo').default and named exports are just other properties. While harder to use from CommonJS, it is the most consistent, which is why we've made it the default behavior.

Another alternative that many people use is add-module-exports which will change the output in the case where your module has only a single default export. It essentially changes export default foo to module.exports = foo; iff there is only a default export and no named exports in the file. This can be nice if CommonJS usability is more important, but it means that if you forget and accidentally add a named export later, you've essentially changed your module from the special mode back to the default, which will almost certainly be a breaking change to the CommonJS interface.

Alternatively, you could always run the files through Babel but continue to use CommonJS directly, disallowing usage of ES6 imports/exports.

I'd say it kind of depends on the usecase. A lot of people like add-module-exports because it avoids the .default, but it has potential edges that can cause problems, and it allows you to potentially write your code such that it may not follow standard ES6 import/export pairings properly.

@michael-ciniawsky
Copy link
Member

The simplest and cleanest would be leaving ES2015 Module transpilation out for the moment Loaders/Plugins are targeting node where CJS will be the recommended module system for some time and two+ active LTS are CJS only regardless of what may come in the near future (approximately 1 year) in terms of ES2015 Modules in node

@joshwiens
Copy link
Member

joshwiens commented Mar 8, 2017

Babel handles this just fine, we just need to have a very clear plan for communicating the change and handle the updates / maintenance in a consistent way across the organization.

This whole topic has been discussed since 2013 and quite frankly, it's time for the community to stop sitting on their hands and get on with this. The language is evolving, everyone simply needs to accept that and evolve along with it.

It is our responsibility as one of the larger organizations in the JavaScript ecosystem to both support the efforts to advance the language and support other organizations like Babel who do so every day.

The priority here is to ensure we are compatible with Webpack, then our other loaders / plugins ( to include the AngularCLI ) & lastly the libs that consume functionality standalone for reason or another.

We are fine with Webpack, have direct control of the second and as far as the last priority, we can proactively convey the change to those libs to include going so far as to pull request in the required updates for the next majors beta tag.

When you distill all of this down, in a CommonJS environment we are really talking about someone having to add a .default to a require('something') which is far from a reason to panic as a consumer of a loader / plugin.

  • As a hard rule, we stay consistent with the handling of ES20whatever with Babel
  • Every defaults update is a semver MAJOR on a beta tag to allow libs time to update / test.
  • As a part of the defaults update for each lib, i'll check currently maintained dependent libs and convey the coming change directly.

@loganfsmyth - Thanks for taking a minute to give us your $0.02 on a topic that I am sure you have discussed to death already :)

@joshwiens
Copy link
Member

Soap box speeches aside, i'm on the fence with all of this for one simple reason...

We don't know enough about how our loaders and plugins are used to make an intelligent decision on this which is a problem we need to solve.

So why don't we split the difference, use webpack-defaults and exclude the use of export default for the initial release and tackle this problem as a separate effort where we can focus on this specific problem.

Upgrading to webpack-defaults will still have to go in as a Major but looking at the amount of work to be done before we could tackle the ES Module problem there would be a reasonable amount of time between Major releases.

@Jessidhia
Copy link

Jessidhia commented Mar 9, 2017

I'd recommend against using add-module-exports at all costs.

If you want to shield your users from having to use .default to access the default export, then do a regular commonjs export.

An alternative is to do something like the following:

  • Use import / export in all your code as usual. No commonjs at all unless you need a dynamic require.
  • Provide a wrapper file that makes your default look like a module.exports.

For example, if your entry point is index.js, you could put this in the package.json:

{
  "module": "./index",
  "main": "./cjs"
}

And then put this in the cjs.js file:

module.exports = require('./index').default

This unfortunately won't be usable with real modules (either you won't be able to require and have to use import(), or you'll be able to require but the result will be a Promise anyway), but that's a problem to think of in 2 years, and will affect even the status quo anyway.

@bebraw
Copy link
Contributor Author

bebraw commented Mar 9, 2017

Yeah, I would be inclined to go with @Kovensky's way. A proxy would allow us to use ES6 modules internally. When the time comes, we can refactor the proxies out.

The coolest thing is that the users won't notice the change unless they happen to refer to code outside of the formal interface. That's actually a potential problem for webpack core as people seem to point below webpack/lib sometimes, but that's another discussion entirely.

@joshwiens
Copy link
Member

I'm fine with that as well. I'd certainly prefer to use ES modules, doing so without having to deal with the onslaught of X doesn't work anymore is a definite bonus.

Unless someone strongly objects, lets go that route and get file-loader updated. We could put the proxy in ./src/index.js generated by defaults on initial upgrade.

@bebraw When you get a chance, make file-loader look the way you want it & i'll update the babel config ( if necessary ) and PR in the template updates to defaults.

@bebraw
Copy link
Contributor Author

bebraw commented Mar 9, 2017

@d3viant0ne Done. We should probably make the following changes to webpack-defaults:

  • Define src/cjs.js that contains module.exports = require('./index').default;.
  • Set package.json "main": "dist/cjs.js",.

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