bug #739 Resolve loaders directly from Encore instead of using their …

…names (Lyrkan)

This PR was merged into the master branch.

Discussion
----------

Resolve loaders directly from Encore instead of using their names

This issue fixes #727 by replacing the use of loader names in rules (for instance `'file-loader'`) by their resolved path (`require.resolve('file-loader')`).

The problem with using their name is that we are assuming that the loader's package will be present at the top-level of `node_modules`.

If for some reason that's not the case and npm/Yarn decides to put it in, let's say `node_modules/@symfony/webpack-encore/node_modules`, the compilation will fail when Webpack tries to require it.

Another reason to do that is to ensure that Webpack uses the right version of a loader. We had some issues before 0.29 because we expected an old version of `file-loader` when some other packages included a newer one that was then used by Webpack because it got hoisted to the top-level `node_modules`.

This change should in my opinion be considered a BC break for people that were:
* adding a different version of a loader we embed into their `package.json`: that one won't be used anymore
* requiring a package that also included one of our embedded loaders: depending on which one was hoisted it could result in a different behavior
* manipulating the generated config and filtering loaders based on their names: the comparison  won't be the same anymore (see the tests I had to change)

I also modified dev dependencies loaders for more consistency but it doesn't really matter for those.

Commits
-------

a3e1e94 Resolve loaders directly from Encore instead of using their names

lib/config-generator.js

@@ -302,7 +302,7 @@ class ConfigGenerator {
 
             rules.push(applyRuleConfigurationCallback('images', {
                 test: /\.(png|jpg|jpeg|gif|ico|svg|webp)$/,
-                loader: loaderName,
+                loader: require.resolve(loaderName),
                 options: loaderOptions
             }));
         }
@@ -330,7 +330,7 @@ class ConfigGenerator {
 
             rules.push(applyRuleConfigurationCallback('fonts', {
                 test: /\.(woff|woff2|ttf|eot|otf)$/,
-                loader: loaderName,
+                loader: require.resolve(loaderName),
                 options: loaderOptions
             }));
         }
@@ -394,7 +394,7 @@ class ConfigGenerator {
         if (this.webpackConfig.useEslintLoader) {
             rules.push(applyRuleConfigurationCallback('eslint', {
                 test: eslintLoaderUtil.getTest(this.webpackConfig),
-                loader: 'eslint-loader',
+                loader: require.resolve('eslint-loader'),
                 exclude: /node_modules/,
                 enforce: 'pre',
                 options: eslintLoaderUtil.getOptions(this.webpackConfig)

lib/loaders/babel.js

@@ -98,7 +98,7 @@ module.exports = {
 
         return [
             {
-                loader: 'babel-loader',
+                loader: require.resolve('babel-loader'),
                 options: babelConfig
             }
         ];

lib/loaders/css-extract.js

@@ -29,7 +29,7 @@ module.exports = {
             // If the CSS extraction is disabled, use the
             // style-loader instead.
             return [{
-                loader: 'style-loader',
+                loader: require.resolve('style-loader'),
                 options: applyOptionsCallback(webpackConfig.styleLoaderConfigurationCallback, options)
 
             }, ...loaders];

lib/loaders/css.js

@@ -41,7 +41,7 @@ module.exports = {
 
         const cssLoaders = [
             {
-                loader: 'css-loader',
+                loader: require.resolve('css-loader'),
                 options: applyOptionsCallback(webpackConfig.cssLoaderConfigurationCallback, options)
             },
         ];
@@ -54,7 +54,7 @@ module.exports = {
             };
 
             cssLoaders.push({
-                loader: 'postcss-loader',
+                loader: require.resolve('postcss-loader'),
                 options: applyOptionsCallback(webpackConfig.postCssLoaderOptionsCallback, postCssLoaderOptions)
             });
         }

lib/loaders/handlebars.js

@@ -25,7 +25,7 @@ module.exports = {
 
         return [
             {
-                loader: 'handlebars-loader',
+                loader: require.resolve('handlebars-loader'),
                 options: applyOptionsCallback(webpackConfig.handlebarsConfigurationCallback, options)
             }
         ];

lib/loaders/less.js

@@ -30,7 +30,7 @@ module.exports = {
         return [
             ...cssLoader.getLoaders(webpackConfig, useCssModules),
             {
-                loader: 'less-loader',
+                loader: require.resolve('less-loader'),
                 options: applyOptionsCallback(webpackConfig.lessLoaderOptionsCallback, config)
             },
         ];

lib/loaders/sass.js

@@ -29,7 +29,7 @@ module.exports = {
             // without this, all url() paths must be relative to the
             // entry file, not the file that contains the url()
             sassLoaders.push({
-                loader: 'resolve-url-loader',
+                loader: require.resolve('resolve-url-loader'),
                 options: Object.assign(
                     {
                         sourceMap: webpackConfig.useSourceMaps
@@ -47,7 +47,7 @@ module.exports = {
         });
 
         sassLoaders.push({
-            loader: 'sass-loader',
+            loader: require.resolve('sass-loader'),
             options: applyOptionsCallback(webpackConfig.sassLoaderOptionsCallback, config)
         });
 

lib/loaders/stylus.js

@@ -30,7 +30,7 @@ module.exports = {
         return [
             ...cssLoader.getLoaders(webpackConfig, useCssModules),
             {
-                loader: 'stylus-loader',
+                loader: require.resolve('stylus-loader'),
                 options: applyOptionsCallback(webpackConfig.stylusLoaderOptionsCallback, config)
             },
         ];

lib/loaders/typescript.js

@@ -51,7 +51,7 @@ module.exports = {
         let loaders = babelLoader.getLoaders(webpackConfig);
         return loaders.concat([
             {
-                loader: 'ts-loader',
+                loader: require.resolve('ts-loader'),
                 // @see https://github.com/TypeStrong/ts-loader/blob/master/README.md#available-options
                 options: config
             }

lib/loaders/vue.js

@@ -25,7 +25,7 @@ module.exports = {
 
         return [
             {
-                loader: 'vue-loader',
+                loader: require.resolve('vue-loader'),
                 options: applyOptionsCallback(webpackConfig.vueLoaderOptionsCallback, options)
             }
         ];

test/config-generator.js

@@ -861,10 +861,10 @@ describe('The config-generator function', () => {
             const actualConfig = configGenerator(config);
 
             const imagesRule = findRule(/\.(png|jpg|jpeg|gif|ico|svg|webp)$/, actualConfig.module.rules);
-            expect(imagesRule.loader).to.equal('file-loader');
+            expect(imagesRule.loader).to.contain('file-loader');
 
             const fontsRule = findRule(/\.(woff|woff2|ttf|eot|otf)$/, actualConfig.module.rules);
-            expect(fontsRule.loader).to.equal('file-loader');
+            expect(fontsRule.loader).to.contain('file-loader');
         });
 
         it('with configureUrlLoader() and missing keys', () => {
@@ -877,10 +877,10 @@ describe('The config-generator function', () => {
             const actualConfig = configGenerator(config);
 
             const imagesRule = findRule(/\.(png|jpg|jpeg|gif|ico|svg|webp)$/, actualConfig.module.rules);
-            expect(imagesRule.loader).to.equal('file-loader');
+            expect(imagesRule.loader).to.contain('file-loader');
 
             const fontsRule = findRule(/\.(woff|woff2|ttf|eot|otf)$/, actualConfig.module.rules);
-            expect(fontsRule.loader).to.equal('file-loader');
+            expect(fontsRule.loader).to.contain('file-loader');
         });
 
         it('with configureUrlLoader()', () => {
@@ -900,12 +900,12 @@ describe('The config-generator function', () => {
             const actualConfig = configGenerator(config);
 
             const imagesRule = findRule(/\.(png|jpg|jpeg|gif|ico|svg|webp)$/, actualConfig.module.rules);
-            expect(imagesRule.loader).to.equal('url-loader');
+            expect(imagesRule.loader).to.contain('url-loader');
             expect(imagesRule.options.name).to.equal('[name].foo.[ext]');
             expect(imagesRule.options.limit).to.equal(8192);
 
             const fontsRule = findRule(/\.(woff|woff2|ttf|eot|otf)$/, actualConfig.module.rules);
-            expect(fontsRule.loader).to.equal('url-loader');
+            expect(fontsRule.loader).to.contain('url-loader');
             expect(fontsRule.options.limit).to.equal(4096);
             expect(fontsRule.options.name).to.equal('[name].bar.[ext]');
         });
@@ -951,7 +951,7 @@ describe('The config-generator function', () => {
             const jsRule = findRule(/\.(jsx?)$/, actualConfig.module.rules);
             expect(String(jsRule.exclude)).to.equal(String(/(node_modules|bower_components)/));
 
-            const babelLoader = jsRule.use.find(loader => loader.loader === 'babel-loader');
+            const babelLoader = jsRule.use.find(loader => /babel-loader/.test(loader.loader));
             const babelEnvPreset = babelLoader.options.presets.find(([name]) => name === '@babel/preset-env');
             expect(babelEnvPreset[1].useBuiltIns).to.equal(false);
         });
@@ -1001,7 +1001,7 @@ describe('The config-generator function', () => {
             const actualConfig = configGenerator(config);
 
             const jsRule = findRule(/\.(jsx?)$/, actualConfig.module.rules);
-            const babelLoader = jsRule.use.find(loader => loader.loader === 'babel-loader');
+            const babelLoader = jsRule.use.find(loader => /babel-loader/.test(loader.loader));
             const babelEnvPreset = babelLoader.options.presets.find(([name]) => name === '@babel/preset-env');
             expect(babelEnvPreset[1].useBuiltIns).to.equal('usage');
             expect(babelEnvPreset[1].corejs).to.equal(3);
@@ -1018,7 +1018,7 @@ describe('The config-generator function', () => {
             const actualConfig = configGenerator(config);
 
             const jsRule = findRule(/\.(jsx?)$/, actualConfig.module.rules);
-            const babelLoader = jsRule.use.find(loader => loader.loader === 'babel-loader');
+            const babelLoader = jsRule.use.find(loader => /babel-loader/.test(loader.loader));
             const babelEnvPreset = babelLoader.options.presets.find(([name]) => name === '@babel/preset-env');
             expect(babelEnvPreset[1].useBuiltIns).to.equal(false);
         });
@@ -1035,7 +1035,7 @@ describe('The config-generator function', () => {
             const actualConfig = configGenerator(config);
 
             const jsRule = findRule(/\.(jsx?)$/, actualConfig.module.rules);
-            const babelLoader = jsRule.use.find(loader => loader.loader === 'babel-loader');
+            const babelLoader = jsRule.use.find(loader => /babel-loader/.test(loader.loader));
             const babelEnvPreset = babelLoader.options.presets.find(([name]) => name === '@babel/preset-env');
             expect(babelEnvPreset[1].useBuiltIns).to.equal('usage');
         });

test/loaders/sass.js

@@ -35,10 +35,10 @@ describe('loaders/sass', () => {
 
         const actualLoaders = sassLoader.getLoaders(config);
         expect(actualLoaders).to.have.lengthOf(2);
-        expect(actualLoaders[0].loader).to.equal('resolve-url-loader');
+        expect(actualLoaders[0].loader).to.contain('resolve-url-loader');
         expect(actualLoaders[0].options.sourceMap).to.be.true;
 
-        expect(actualLoaders[1].loader).to.equal('sass-loader');
+        expect(actualLoaders[1].loader).to.contain('sass-loader');
         expect(actualLoaders[1].options.sourceMap).to.be.true;
         expect(cssLoaderStub.getCall(0).args[1]).to.be.false;
 
@@ -55,10 +55,10 @@ describe('loaders/sass', () => {
 
         const actualLoaders = sassLoader.getLoaders(config);
         expect(actualLoaders).to.have.lengthOf(2);
-        expect(actualLoaders[0].loader).to.equal('resolve-url-loader');
+        expect(actualLoaders[0].loader).to.contain('resolve-url-loader');
         expect(actualLoaders[0].options.sourceMap).to.be.false;
 
-        expect(actualLoaders[1].loader).to.equal('sass-loader');
+        expect(actualLoaders[1].loader).to.contain('sass-loader');
         // sourcemaps always enabled when resolve-url-loader is enabled
         expect(actualLoaders[1].options.sourceMap).to.be.true;
 
@@ -79,7 +79,7 @@ describe('loaders/sass', () => {
 
         const actualLoaders = sassLoader.getLoaders(config);
         expect(actualLoaders).to.have.lengthOf(2);
-        expect(actualLoaders[0].loader).to.equal('resolve-url-loader');
+        expect(actualLoaders[0].loader).to.contain('resolve-url-loader');
         expect(actualLoaders[0].options.removeCR).to.be.true;
 
         cssLoader.getLoaders.restore();
@@ -98,7 +98,7 @@ describe('loaders/sass', () => {
 
         const actualLoaders = sassLoader.getLoaders(config);
         expect(actualLoaders).to.have.lengthOf(1);
-        expect(actualLoaders[0].loader).to.equal('sass-loader');
+        expect(actualLoaders[0].loader).to.contain('sass-loader');
         expect(actualLoaders[0].options.sourceMap).to.be.false;
 
         cssLoader.getLoaders.restore();
@@ -158,10 +158,10 @@ describe('loaders/sass', () => {
 
         const actualLoaders = sassLoader.getLoaders(config, true);
         expect(actualLoaders).to.have.lengthOf(2);
-        expect(actualLoaders[0].loader).to.equal('resolve-url-loader');
+        expect(actualLoaders[0].loader).to.contain('resolve-url-loader');
         expect(actualLoaders[0].options.sourceMap).to.be.true;
 
-        expect(actualLoaders[1].loader).to.equal('sass-loader');
+        expect(actualLoaders[1].loader).to.contain('sass-loader');
         expect(actualLoaders[1].options.sourceMap).to.be.true;
         expect(cssLoaderStub.getCall(0).args[1]).to.be.true;