Skip to content
This repository was archived by the owner on Mar 17, 2021. It is now read-only.

refactor: export as ES2015 Module#10

Merged
joshwiens merged 1 commit into
webpack-contrib:masterfrom
SpaceK33z:patch-1
Oct 1, 2017
Merged

refactor: export as ES2015 Module#10
joshwiens merged 1 commit into
webpack-contrib:masterfrom
SpaceK33z:patch-1

Conversation

@SpaceK33z

Copy link
Copy Markdown
Contributor

Since webpack v2, it's better to use ES modules. Note that this is incompatible with webpack v1, so it should be released as a major version.

Since webpack v2, it's better to use ES modules. Note that this is incompatible with webpack v1, so it should be released as a major version.
@bondz

bondz commented Feb 27, 2017

Copy link
Copy Markdown

Is this going to be merged?

@bebraw

bebraw commented Mar 5, 2017

Copy link
Copy Markdown

It would be better to go with

// webpack 2
if (loader.version === 2) return `export default ${publicPath}`

return `module.exports = ${publicPath}`

or so. We need a standard way for checking which version of webpack is used.

Related discussion at webpack-contrib/file-loader#130 .

@michael-ciniawsky

Copy link
Copy Markdown
Member

@SpaceK33z @bebraw There are few PR open from @SpaceK33z which started/proposed es2015 modules exports, the solution above it what worked for me, but would be good to be approved by @sokra 😛

Comment thread index.js
@@ -5,5 +5,5 @@
module.exports = function(content) {
this.cacheable && this.cacheable();
this.value = content;

@michael-ciniawsky michael-ciniawsky Mar 6, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated to the PR but what is this line actually doing 🤔

this.value = content

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. It's some webpack 0.9 fix from 9bbe8b2 . I wonder if that line is even needed now.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated, but from my debugging session, this assignment does not actually occur, as afterwards, this.value remains undefined. Or, actually, not defined at all.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another note. this.cacheable is set by default now so it would be possible to drop that.

@bebraw

bebraw commented Mar 6, 2017

Copy link
Copy Markdown

Can you sign the CLA? It has to be signed separately for the contrib org. That + fallback for webpack 1 + possible removal of this.value bit after that is understood would make it possible to merge this.

@bebraw

bebraw commented Mar 6, 2017

Copy link
Copy Markdown

We had a quick chat about this. Basically return "export default " + JSON.stringify(content); is enough. You should, however, set a peer dependency against webpack 2 and publish the change as a major version.

Branching would bring unnecessary trouble so a clean break is neater for the users. If you go webpack 2, better go all the way.

Comment thread index.js
this.cacheable && this.cacheable();
this.value = content;
return "module.exports = " + JSON.stringify(content);
return "export default " + JSON.stringify(content);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this why it isn't working for me with import foo from './foo'?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to do

import * as foo from './foo'

which is very wrong... If this will correct behavior, please accept...

Confirmed working with webpack v2.2.1.

@mightyiam mightyiam left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me with babel v2.2.1.

Comment thread index.js
this.cacheable && this.cacheable();
this.value = content;
return "module.exports = " + JSON.stringify(content);
return "export default " + JSON.stringify(content);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to do

import * as foo from './foo'

which is very wrong... If this will correct behavior, please accept...

Confirmed working with webpack v2.2.1.

@michael-ciniawsky

Copy link
Copy Markdown
Member

@mightyiam Thats possible 😛 did you disable babel modules transformations?

@SpaceK33z

Copy link
Copy Markdown
Contributor Author

@bebraw I signed the CLA. Is this safe to merge now?

@bebraw
bebraw requested a review from joshwiens March 7, 2017 17:42
@bebraw

bebraw commented Mar 7, 2017

Copy link
Copy Markdown

@SpaceK33z Let's wait for a review from Joshua. It's a simple change, but better play it safe.

@joshwiens

joshwiens commented Mar 7, 2017

Copy link
Copy Markdown
Member

Squash & merge with the following commit message ( matters for defaults changelog stuff )

feat: Export as ES Module

BREAKING CHANGE: ES Modules are not compatible with Webpack v1.x

Worth noting, this should be released in conjunction with the webpack-defaults update as it enforces node > 4.3 and is technically breaking as well.

Added to milestone 1.0.0-beta.0 and should be published as the beta tag to be safe ( silly for 6 lines of code, I know ).

@mightyiam

Copy link
Copy Markdown

@michael-ciniawsky I wasn't using babel. I wasn't combining it with any other loader, if that's what you mean.

@michael-ciniawsky

michael-ciniawsky commented Mar 7, 2017

Copy link
Copy Markdown
Member

@mightyiam Yep, that's what I meant, forget about it then 😛

@joshwiens

joshwiens commented Mar 7, 2017

Copy link
Copy Markdown
Member

@bebraw - Pretty sure @mightyiam is talking about doing a standard named import as opposed to doing import * as

@mightyiam - That will require a slightly different set of changes that will happen as a part of webpack-defaults update and be included in the same MAJOR version release as the ES module change here.

After defaults, the transpiled code would look something like this ...

"use strict";

Object.defineProperty(exports, "__esModule", {
  value: true
});

exports.default = function (content) {
  this.cacheable && this.cacheable();
  this.value = content;
  return `export default ${JSON.stringify(content)}`;
};

@joshwiens joshwiens added this to the 1.0.0-beta.0 milestone Mar 7, 2017
@joshwiens joshwiens self-assigned this Mar 8, 2017
@mightyiam

Copy link
Copy Markdown

Is something blocking this?

@bebraw

bebraw commented Mar 14, 2017

Copy link
Copy Markdown

@mightyiam Not really. Just busy times for a lot of people.

@joshwiens

Copy link
Copy Markdown
Member

@mightyiam - This has to be released as a semver MAJOR, we have another breaking change that needs to be merge ready before this is merged & released.

@joshwiens joshwiens removed their assignment Aug 17, 2017
@mightyiam

Copy link
Copy Markdown

Any update?

@michael-ciniawsky michael-ciniawsky changed the title Export an ES Module instead of CommonJS module feat: export as ES2015 Module Sep 27, 2017
@michael-ciniawsky michael-ciniawsky changed the title feat: export as ES2015 Module refactor: export as ES2015 Module Sep 27, 2017
@joshwiens
joshwiens merged commit 215fba7 into webpack-contrib:master Oct 1, 2017
@joshwiens

Copy link
Copy Markdown
Member

@mightyiam - This will still require #25 before it finds it's way out to npm. I'll get that PR updates shortly for a final once over.

@mightyiam

Copy link
Copy Markdown

Thank you!

joshwiens pushed a commit that referenced this pull request Nov 20, 2017
feat: Export as ES Module

BREAKING CHANGE: ES Modules are not compatible with Webpack v1.x
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants