Remove automatic -loader module name extension #2986

Closed
jhnns opened this Issue Sep 12, 2016 · 30 comments

Comments

@jhnns
Member

jhnns commented Sep 12, 2016

I'm submitting a feature request

Webpack version:
2.x

Current behavior:
webpack automatically appends -loader to a module when the first module lookup failed

Expected/desired behavior:
webpack should not append -loader

What is the motivation / use case for changing the behavior?
It can lead to errors that are hard to understand for newcomers. I've seen it a couple of times that webpack tried to resolve the css module as css-loader because people have just configured loader: "css". In combination with npm@3, this can be very confusing as you may have never installed the css module by yourself.

This change would probably require a lot of webpack configs to be adjusted, but since the loader configuration has been changed in webpack@2 anyway, we should include that change as well. As a downside, the config will become a little more verbose because now everyone has to write style-loader!css-loader, but I think the benefit outweighs the downside by far.

@TheLarkInn

This comment has been minimized.

Show comment
Hide comment
@TheLarkInn

TheLarkInn Sep 12, 2016

Member

This makes sense. Its not too much extra work to have someone add "-loader". Explicit.

Member

TheLarkInn commented Sep 12, 2016

This makes sense. Its not too much extra work to have someone add "-loader". Explicit.

@bebraw

This comment has been minimized.

Show comment
Hide comment
@bebraw

bebraw Sep 12, 2016

Member

Yeah, this would be a good change for webpack 2. Less confusion.

Member

bebraw commented Sep 12, 2016

Yeah, this would be a good change for webpack 2. Less confusion.

@bebraw bebraw referenced this issue in webpack/webpack.js.org Sep 14, 2016

Merged

Configuration Documentation #157

@sokra sokra added this to the 2.1-RC Blocking milestone Sep 14, 2016

@amsdamsgram

This comment has been minimized.

Show comment
Hide comment
@amsdamsgram

amsdamsgram Sep 15, 2016

Hi, I'm willing to take a look at this if no one is already on it!

Hi, I'm willing to take a look at this if no one is already on it!

@TheLarkInn

This comment has been minimized.

Show comment
Hide comment
@TheLarkInn

TheLarkInn Sep 15, 2016

Member

Go right ahead.

Member

TheLarkInn commented Sep 15, 2016

Go right ahead.

@TheLarkInn

This comment has been minimized.

Show comment
Hide comment
@TheLarkInn

TheLarkInn Sep 15, 2016

Member

This would however land after webpack 2.x as it would be a pretty irritating breaking change unless anyone disagrees?

Member

TheLarkInn commented Sep 15, 2016

This would however land after webpack 2.x as it would be a pretty irritating breaking change unless anyone disagrees?

@amsdamsgram

This comment has been minimized.

Show comment
Hide comment
@amsdamsgram

amsdamsgram Sep 15, 2016

No worries.
Where should I start looking at?

No worries.
Where should I start looking at?

@jhnns

This comment has been minimized.

Show comment
Hide comment
@jhnns

jhnns Sep 15, 2016

Member

@iDams awesome! 👍
@TheLarkInn This would be a change that requires a lot of webpack.configs to be adjusted. But: Since we are changing the way how loaders are configured in webpack 2 anyway, we should put this change into webpack 2.x

Member

jhnns commented Sep 15, 2016

@iDams awesome! 👍
@TheLarkInn This would be a change that requires a lot of webpack.configs to be adjusted. But: Since we are changing the way how loaders are configured in webpack 2 anyway, we should put this change into webpack 2.x

@amsdamsgram

This comment has been minimized.

Show comment
Hide comment
@amsdamsgram

amsdamsgram Sep 16, 2016

Hey, I could use some advice :D
I've started looking into /lib files especially those which seems to be linked to module like ModuleParseHelper.js, RawModule.js, ... Am I in going into the right direction?

Hey, I could use some advice :D
I've started looking into /lib files especially those which seems to be linked to module like ModuleParseHelper.js, RawModule.js, ... Am I in going into the right direction?

@andreypopp

This comment has been minimized.

Show comment
Hide comment
@amsdamsgram

This comment has been minimized.

Show comment
Hide comment
@amsdamsgram

This comment has been minimized.

Show comment
Hide comment
@amsdamsgram

amsdamsgram Sep 16, 2016

So I think removing this line will remove automatic "-loader" but I would like to test it to be sure and also to see if everything still works fine.
How can I test?

I tried to run npm test but there are 7 tests failed and 32 pending on master without any changes yet.
Is it normal?

So I think removing this line will remove automatic "-loader" but I would like to test it to be sure and also to see if everything still works fine.
How can I test?

I tried to run npm test but there are 7 tests failed and 32 pending on master without any changes yet.
Is it normal?

@jhnns

This comment has been minimized.

Show comment
Hide comment
@jhnns

jhnns Sep 16, 2016

Member

If there are already failing tests on master, you can ignore them. Just check if your changes don't break additional tests.

@sokra failing tests on the main branch make contributing a lot harder. We should try to keep failing tests only on feature branches. Have we ever talked about using git flow? :)

Member

jhnns commented Sep 16, 2016

If there are already failing tests on master, you can ignore them. Just check if your changes don't break additional tests.

@sokra failing tests on the main branch make contributing a lot harder. We should try to keep failing tests only on feature branches. Have we ever talked about using git flow? :)

@amsdamsgram

This comment has been minimized.

Show comment
Hide comment
@amsdamsgram

amsdamsgram Sep 16, 2016

Thanks @jhnns, I will check.

But how can I actually test that webpack doesn't actually add "-loader" to a module name?
Because for now, I am just assuming it won't :/

I tried to run node build.js in examples/code-splitted-css-bundle/ but I got the following error
capture d ecran 2016-09-16 a 16 40 27

My goal was to replace css-loader by css in the webpack.config.js file and check that webpack was not adding "-loader" anymore.

Thanks @jhnns, I will check.

But how can I actually test that webpack doesn't actually add "-loader" to a module name?
Because for now, I am just assuming it won't :/

I tried to run node build.js in examples/code-splitted-css-bundle/ but I got the following error
capture d ecran 2016-09-16 a 16 40 27

My goal was to replace css-loader by css in the webpack.config.js file and check that webpack was not adding "-loader" anymore.

@jhnns

This comment has been minimized.

Show comment
Hide comment
@jhnns

jhnns Sep 16, 2016

Member

Looks like the example is broken 😁

There are several ways how to test this. I think the easiest way would be to use css in a webpack.config.js and then to check for an error (because just css should not work anymore).

Member

jhnns commented Sep 16, 2016

Looks like the example is broken 😁

There are several ways how to test this. I think the easiest way would be to use css in a webpack.config.js and then to check for an error (because just css should not work anymore).

@jhnns

This comment has been minimized.

Show comment
Hide comment
@jhnns

jhnns Sep 16, 2016

Member

If you have troubles writing the test, we can also omit I think. Tests should usually check for actual behavior, not for behavior that used to be there.

Member

jhnns commented Sep 16, 2016

If you have troubles writing the test, we can also omit I think. Tests should usually check for actual behavior, not for behavior that used to be there.

@sokra

This comment has been minimized.

Show comment
Hide comment
@sokra

sokra Sep 18, 2016

Member

@iDams you can add a test case in test/configCases for this. Just check if an error is emitted by webpack for resolving (this is possible with a errors.js in the test directory, see other test cases).

Member

sokra commented Sep 18, 2016

@iDams you can add a test case in test/configCases for this. Just check if an error is emitted by webpack for resolving (this is possible with a errors.js in the test directory, see other test cases).

@sokra

This comment has been minimized.

Show comment
Hide comment
@sokra

sokra Sep 18, 2016

Member

In the example the generation of the README fails, because the build fails. Looks fine. You can also run the examples with node ../../bin/webpack.js example.js js/output.js or node ../../bin/webpack.js depending on the example (check if entry and output is in webpack.config.js).

Member

sokra commented Sep 18, 2016

In the example the generation of the README fails, because the build fails. Looks fine. You can also run the examples with node ../../bin/webpack.js example.js js/output.js or node ../../bin/webpack.js depending on the example (check if entry and output is in webpack.config.js).

@sokra

This comment has been minimized.

Show comment
Hide comment
@sokra

sokra Sep 18, 2016

Member

I tried to run npm test but there are 7 tests failed and 32 pending on master without any changes yet.
Is it normal?

no, it should work. Compare your result to the travis build: https://travis-ci.org/webpack/webpack

Check if you done the setup correctly: https://github.com/webpack/webpack/blob/master/CONTRIBUTING.md#setup
The npm link stuff is important.

Member

sokra commented Sep 18, 2016

I tried to run npm test but there are 7 tests failed and 32 pending on master without any changes yet.
Is it normal?

no, it should work. Compare your result to the travis build: https://travis-ci.org/webpack/webpack

Check if you done the setup correctly: https://github.com/webpack/webpack/blob/master/CONTRIBUTING.md#setup
The npm link stuff is important.

@amsdamsgram

This comment has been minimized.

Show comment
Hide comment
@amsdamsgram

amsdamsgram Sep 19, 2016

@jhnns @sokra Thanks, I will try to add a test in configCases + look why I have errors on master

@jhnns @sokra Thanks, I will try to add a test in configCases + look why I have errors on master

@amsdamsgram

This comment has been minimized.

Show comment
Hide comment
@amsdamsgram

amsdamsgram Sep 22, 2016

Still have failed tests on master with a correct setup

I have done the setup correctly now. Thanks @sokra for pointing the npm link stuff. I didn't do it.
But even on master, when I run npm test, I have 2 failed tests.

capture d ecran 2016-09-22 a 16 58 19

Lost of failed tests with my changes

And when I remove this line
I have more errors, and npm test doesn't finish properly.
I get a npm ERR! Test failed..
I have huge errors like this:

capture d ecran 2016-09-22 a 17 06 12

All the loaders tests failed with kind of the same error like in
require("raw!...") or require(css!...) etc. I get a can't resolve raw or css

Any input on this?
I will keep looking how this is related to the line I removed!

Still have failed tests on master with a correct setup

I have done the setup correctly now. Thanks @sokra for pointing the npm link stuff. I didn't do it.
But even on master, when I run npm test, I have 2 failed tests.

capture d ecran 2016-09-22 a 16 58 19

Lost of failed tests with my changes

And when I remove this line
I have more errors, and npm test doesn't finish properly.
I get a npm ERR! Test failed..
I have huge errors like this:

capture d ecran 2016-09-22 a 17 06 12

All the loaders tests failed with kind of the same error like in
require("raw!...") or require(css!...) etc. I get a can't resolve raw or css

Any input on this?
I will keep looking how this is related to the line I removed!

@jhnns

This comment has been minimized.

Show comment
Hide comment
@jhnns

jhnns Oct 4, 2016

Member

Any input on this?
I will keep looking how this is related to the line I removed!

This is exactly why removing this line is a breaking change. You need to adjust the failing test cases, they should use val-loader!... instead of just using val!...

Member

jhnns commented Oct 4, 2016

Any input on this?
I will keep looking how this is related to the line I removed!

This is exactly why removing this line is a breaking change. You need to adjust the failing test cases, they should use val-loader!... instead of just using val!...

@amsdamsgram

This comment has been minimized.

Show comment
Hide comment
@amsdamsgram

amsdamsgram Oct 5, 2016

Got it, thanks!

Got it, thanks!

@amsdamsgram amsdamsgram referenced this issue Oct 6, 2016

Merged

Remove automatic -loader module name extension #3102

2 of 11 tasks complete
@amsdamsgram

This comment has been minimized.

Show comment
Hide comment
@amsdamsgram

amsdamsgram Oct 7, 2016

Hey, I submitted a PR.

All tests passed on my local but travis-ci on Linux with node.js 5 failed.
capture d ecran 2016-10-07 a 10 28 44

Do you have any advice on how to debug this?

I've seen couple of options, VM, Travis Build, Vagrant or with a docker image.
I'm on OSX so I will probably have to install a VM with Linux.

Hey, I submitted a PR.

All tests passed on my local but travis-ci on Linux with node.js 5 failed.
capture d ecran 2016-10-07 a 10 28 44

Do you have any advice on how to debug this?

I've seen couple of options, VM, Travis Build, Vagrant or with a docker image.
I'm on OSX so I will probably have to install a VM with Linux.

@jhnns

This comment has been minimized.

Show comment
Hide comment
Member

jhnns commented Oct 19, 2016

cecilemuller added a commit to wildpeaks/archived--boilerplate-webpack2-es2015 that referenced this issue Oct 22, 2016

Included "-loader" in the name of every loader
Because it will be required if this change passes:
webpack/webpack#2986

jhnns added a commit to webpack/webpack.js.org that referenced this issue Oct 25, 2016

Add `-loader` to every loader name
Aligns with breaking change introduced with webpack/webpack#3102
See webpack/webpack#2986

@jhnns jhnns referenced this issue in webpack/webpack.js.org Oct 25, 2016

Merged

Add `-loader` to every loader name #279

@jhnns jhnns closed this Oct 26, 2016

@joshka joshka referenced this issue in webpack/webpack.js.org Nov 2, 2016

Closed

feat(loaders): add loaders content #249

@michael-ciniawsky michael-ciniawsky referenced this issue in postcss/postcss-loader Nov 5, 2016

Closed

Issue with sass and postcss/autoprefixer #74

@wesleycho wesleycho referenced this issue in tamasd/font-awesome-webpack2 Nov 14, 2016

Merged

fix: change to -loader due to Webpack 2 change #2

@wesleycho

This comment has been minimized.

Show comment
Hide comment
@wesleycho

wesleycho Nov 14, 2016

I would like to add that this was a frustrating breaking change that appears to have fairly weak rationale behind it relative to the cost - having to chase around what changed for an hour and filing PRs to fix Webpack plugin projects was not fun.

I would like to add that this was a frustrating breaking change that appears to have fairly weak rationale behind it relative to the cost - having to chase around what changed for an hour and filing PRs to fix Webpack plugin projects was not fun.

@TheLarkInn

This comment has been minimized.

Show comment
Hide comment
@TheLarkInn

TheLarkInn Nov 14, 2016

Member

I apologize for the this @wesleycho. This change is one of many steps that plan to pave the way for overall more consistent and standard config usage.

This was something that would have had to been cherry picked out and then back into master if we tried to avoid it. I will create an issue that will block 2.x release until we have a great error message that will help people migration through this change.

Alternatively we would welcome any suggestions for this as well. Catching the 100x ways to specify loaders has made it very hard to catch and consistently error (which is why we don't have a PR merged for this yet).

Member

TheLarkInn commented Nov 14, 2016

I apologize for the this @wesleycho. This change is one of many steps that plan to pave the way for overall more consistent and standard config usage.

This was something that would have had to been cherry picked out and then back into master if we tried to avoid it. I will create an issue that will block 2.x release until we have a great error message that will help people migration through this change.

Alternatively we would welcome any suggestions for this as well. Catching the 100x ways to specify loaders has made it very hard to catch and consistently error (which is why we don't have a PR merged for this yet).

@sokra

This comment has been minimized.

Show comment
Hide comment
@sokra

sokra Nov 14, 2016

Member

Here you are: #3289

Member

sokra commented Nov 14, 2016

Here you are: #3289

@danez danez referenced this issue in babel/babel-loader Nov 16, 2016

Merged

Use babel-loader instead of babel in README #330

@simonbuchan

This comment has been minimized.

Show comment
Hide comment
@simonbuchan

simonbuchan Jan 18, 2017

Hmm, I expected that resolveLoader.moduleExtensions would work the same as resolve.extensions (used to): and try each appended to the request in order. i.e, it defaulted to moduleExtensions: ['-loader', ''] and would try css-loader first, then css

I suspect the breaking-changeness of this could be walked back in a 2.3+ safely enough by making it work that way, which should only break if someone has some-loader-loader but expected to find some-loader exactly - npms.io isn't great at finding module name patterns, but I couldn't see any such names.

With the change to resolve.extensions though, I'm not sure that it still works that way? E.g. try all the extensions first? https://webpack.js.org/configuration/resolve/ isn't clear on what happens with conflicts.

Hmm, I expected that resolveLoader.moduleExtensions would work the same as resolve.extensions (used to): and try each appended to the request in order. i.e, it defaulted to moduleExtensions: ['-loader', ''] and would try css-loader first, then css

I suspect the breaking-changeness of this could be walked back in a 2.3+ safely enough by making it work that way, which should only break if someone has some-loader-loader but expected to find some-loader exactly - npms.io isn't great at finding module name patterns, but I couldn't see any such names.

With the change to resolve.extensions though, I'm not sure that it still works that way? E.g. try all the extensions first? https://webpack.js.org/configuration/resolve/ isn't clear on what happens with conflicts.

@waltcow waltcow referenced this issue in waltcow/blog Mar 2, 2017

Open

Webpack 2 升级指南和特性 #13

@jcrben

This comment has been minimized.

Show comment
Hide comment
@jcrben

jcrben Apr 10, 2017

Next time, I suggest that instead of removing something like this in 2.x, put a warning in 2.x and remove it in 3.x.

jcrben commented Apr 10, 2017

Next time, I suggest that instead of removing something like this in 2.x, put a warning in 2.x and remove it in 3.x.

@cmkshaw cmkshaw referenced this issue in posabsolute/redux-flash-notification Apr 19, 2017

Open

Updated refs to loaders in webpack config & component for webpack 2.0 #10

@hzlmn

This comment has been minimized.

Show comment
Hide comment
@hzlmn

hzlmn May 19, 2017

@jcrben It's the way how @postcss do it. Webpack team should learn more about convenient releases from it.

hzlmn commented May 19, 2017

@jcrben It's the way how @postcss do it. Webpack team should learn more about convenient releases from it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment