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

refactor: export as ES2015 Module #10

Merged
merged 1 commit into from
Oct 1, 2017

Conversation

SpaceK33z
Copy link
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
Copy link

bondz commented Feb 27, 2017

Is this going to be merged?

@bebraw
Copy link

bebraw commented Mar 5, 2017

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
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 😛

@@ -5,5 +5,5 @@
module.exports = function(content) {
this.cacheable && this.cacheable();
this.value = content;
Copy link
Member

@michael-ciniawsky michael-ciniawsky Mar 6, 2017

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

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.

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

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
Copy link

bebraw commented Mar 6, 2017

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
Copy link

bebraw commented Mar 6, 2017

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.

@@ -5,5 +5,5 @@
module.exports = function(content) {
this.cacheable && this.cacheable();
this.value = content;
return "module.exports = " + JSON.stringify(content);
return "export default " + JSON.stringify(content);

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'?

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.

Copy link

@mightyiam mightyiam left a comment

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.

@@ -5,5 +5,5 @@
module.exports = function(content) {
this.cacheable && this.cacheable();
this.value = content;
return "module.exports = " + JSON.stringify(content);
return "export default " + JSON.stringify(content);

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
Member

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

@SpaceK33z
Copy link
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
Copy link

bebraw commented Mar 7, 2017

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

@joshwiens
Copy link
Member

joshwiens commented Mar 7, 2017

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

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

@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Mar 7, 2017

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

@joshwiens
Copy link
Member

joshwiens commented Mar 7, 2017

@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

Is something blocking this?

@bebraw
Copy link

bebraw commented Mar 14, 2017

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

@joshwiens
Copy link
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

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
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

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