refactor: export as ES2015 Module#10
Conversation
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.
|
Is this going to be merged? |
|
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 . |
|
@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; | |||
There was a problem hiding this comment.
Unrelated to the PR but what is this line actually doing 🤔
this.value = contentThere was a problem hiding this comment.
Good question. It's some webpack 0.9 fix from 9bbe8b2 . I wonder if that line is even needed now.
There was a problem hiding this comment.
Unrelated, but from my debugging session, this assignment does not actually occur, as afterwards, this.value remains undefined. Or, actually, not defined at all.
There was a problem hiding this comment.
Another note. this.cacheable is set by default now so it would be possible to drop that.
|
Can you sign the CLA? It has to be signed separately for the contrib org. That + fallback for webpack 1 + possible removal of |
|
We had a quick chat about this. Basically Branching would bring unnecessary trouble so a clean break is neater for the users. If you go webpack 2, better go all the way. |
| this.cacheable && this.cacheable(); | ||
| this.value = content; | ||
| return "module.exports = " + JSON.stringify(content); | ||
| return "export default " + JSON.stringify(content); |
There was a problem hiding this comment.
Is this why it isn't working for me with import foo from './foo'?
There was a problem hiding this comment.
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.
| this.cacheable && this.cacheable(); | ||
| this.value = content; | ||
| return "module.exports = " + JSON.stringify(content); | ||
| return "export default " + JSON.stringify(content); |
There was a problem hiding this comment.
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 Thats possible 😛 did you disable babel modules transformations? |
|
@bebraw I signed the CLA. Is this safe to merge now? |
|
@SpaceK33z Let's wait for a review from Joshua. It's a simple change, but better play it safe. |
|
Squash & merge with the following commit message ( matters for defaults changelog stuff ) Worth noting, this should be released in conjunction with the Added to milestone |
|
@michael-ciniawsky I wasn't using babel. I wasn't combining it with any other loader, if that's what you mean. |
|
@mightyiam Yep, that's what I meant, forget about it then 😛 |
|
@bebraw - Pretty sure @mightyiam is talking about doing a standard named import as opposed to doing @mightyiam - That will require a slightly different set of changes that will happen as a part of After defaults, the transpiled code would look something like this ... |
|
Is something blocking this? |
|
@mightyiam Not really. Just busy times for a lot of people. |
|
@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. |
|
Any update? |
|
@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. |
|
Thank you! |
feat: Export as ES Module BREAKING CHANGE: ES Modules are not compatible with Webpack v1.x
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.