bug #462 Always used named chunks AND prevent split chunks from changing too often. (weaverryan)

This PR was squashed before being merged into the master branch (closes #462).

Discussion
----------

Always used named chunks AND prevent split chunks from changing too often.

Fixes #461

But, this does not expose the entry names internally in the code (as these are used instead of auto-incremet ids). I don't think there's a way around that.

Also, I'm still tracking down a somewhat related but that causes a related issue (meaningless id's changing in generated code) under very specific conditions. In fact, you can see it in this test case just by changing all the 3 of the entries to an array - e.g.

```js
config.addEntry('main1', ['./js/code_splitting']);
if (includeExtraEntry) {
    config.addEntry('main2', ['./js/eslint']);
}
config.addEntry('main3', ['./js/no_require']);
```

That doesn't need to block this PR, but I need to figure out the issue (it's Webpack behavior) and see if it affects versioning (i.e. if the contents change but the file hashes do NOT change, this is a BIG problem, otherwise, it's more of a minor, performance problem as filenames would just be changing too often).

Commits
-------

a8b6725 Making how the splitting works more consistent
4de5536 Fixing tests
13e07c6 linking to issue
7c96476 Fixing a bug where split filenames might change too often
a426b8c Always used named chunks to avoid caching problems

Author: weaverryan

lib/config-generator.js

@@ -403,27 +403,22 @@ class ConfigGenerator {
             // which can be useful for debugging, especially with HMR
             optimization.namedModules = true;
         }
+        // https://github.com/webpack/webpack/issues/8354
+        // likely can be removed in Webpack 5
+        // https://github.com/webpack/webpack/pull/8374
+        optimization.chunkIds = 'named';
 
         let splitChunks = {
             chunks: this.webpackConfig.shouldSplitEntryChunks ? 'all' : 'async'
         };
 
-        // hash the split filenames in production to protect exposing entry names
+        // This causes the split filenames (& internal names) to be,
+        // for example, 0.js. This is needed so that the filename
+        // doesn't change suddenly when another entry needs that same
+        // shared code (e.g. vendor~entry1~entry2.js).
+        // https://github.com/webpack/webpack/issues/8426#issuecomment-442375207
         if (this.webpackConfig.shouldSplitEntryChunks && this.webpackConfig.isProduction()) {
-            // taken from SplitChunksPlugin
-            const hashFilename = name => {
-                return crypto
-                    .createHash('md4')
-                    .update(name)
-                    .digest('hex')
-                    .slice(0, 8);
-            };
-
-            splitChunks.name = function(module, chunks, cacheGroup) {
-                const names = chunks.map(c => c.name);
-
-                return cacheGroup + '~' + hashFilename(names.join('~'));
-            };
+            splitChunks.name = false;
         }
 
         if (this.webpackConfig.sharedCommonsEntryName) {

test/config-generator.js

@@ -877,7 +877,7 @@ describe('The config-generator function', () => {
 
             const actualConfig = configGenerator(config);
             expect(actualConfig.optimization.splitChunks.chunks).to.equal('all');
-            expect(actualConfig.optimization.splitChunks.name).to.be.a('function');
+            expect(actualConfig.optimization.splitChunks.name).to.be.false;
         });
     });
 

test/functional.js

@@ -31,9 +31,7 @@ function createWebpackConfig(outputDirName = '', command, argv = {}) {
 }
 
 function convertToManifestPath(assetSrc, webpackConfig) {
-    const manifestData = JSON.parse(
-        fs.readFileSync(path.join(webpackConfig.outputPath, 'manifest.json'), 'utf8')
-    );
+    const manifestData = JSON.parse(readOutputFileContents('manifest.json', webpackConfig));
 
     if (typeof manifestData[assetSrc] === 'undefined') {
         throw new Error(`Path ${assetSrc} not found in manifest!`);
@@ -42,6 +40,26 @@ function convertToManifestPath(assetSrc, webpackConfig) {
     return manifestData[assetSrc];
 }
 
+function readOutputFileContents(filename, config) {
+    const fullPath = path.join(config.outputPath, filename);
+
+    if (!fs.existsSync(fullPath)) {
+        throw new Error(`Output file "${filename}" does not exist.`);
+    }
+
+    return fs.readFileSync(fullPath, 'utf8');
+}
+
+function getEntrypointData(config, entryName) {
+    const entrypointsData = JSON.parse(readOutputFileContents('entrypoints.json', config));
+
+    if (typeof entrypointsData.entrypoints[entryName] === 'undefined') {
+        throw new Error(`The entry ${entryName} was not found!`);
+    }
+
+    return entrypointsData.entrypoints[entryName];
+}
+
 describe('Functional tests using webpack', function() {
     // being functional tests, these can take quite long
     this.timeout(8000);
@@ -318,6 +336,22 @@ describe('Functional tests using webpack', function() {
             });
         });
 
+        it('.mjs files are supported natively', (done) => {
+            const config = createWebpackConfig('web/build', 'dev');
+            config.addEntry('main', './js/hello_world');
+            config.setPublicPath('/build');
+
+            testSetup.runWebpack(config, (webpackAssert) => {
+                // check that main.js has the correct contents
+                webpackAssert.assertOutputFileContains(
+                    'main.js',
+                    'Hello World!'
+                );
+
+                done();
+            });
+        });
+
         describe('addStyleEntry .js files are removed', () => {
             it('Without versioning', (done) => {
                 const config = createWebpackConfig('web', 'dev');
@@ -1628,7 +1662,7 @@ module.exports = {
             });
         });
 
-        describe('entrypoints.json', () => {
+        describe('entrypoints.json & splitChunks()', () => {
             it('Use "all" splitChunks & look at entrypoints.json', (done) => {
                 const config = createWebpackConfig('web/build', 'dev');
                 config.addEntry('main', ['./css/roboto_font.css', './js/no_require', 'vue']);
@@ -1760,18 +1794,18 @@ module.exports = {
                     webpackAssert.assertOutputJsonFileMatches('entrypoints.json', {
                         entrypoints: {
                             main: {
-                                js: ['/build/runtime.js', '/build/vendors~cc515e6e.js', '/build/default~cc515e6e.js', '/build/main.js'],
-                                css: ['/build/default~cc515e6e.css']
+                                js: ['/build/runtime.js', '/build/0.js', '/build/1.js', '/build/main.js'],
+                                css: ['/build/1.css']
                             },
                             other: {
-                                js: ['/build/runtime.js', '/build/vendors~cc515e6e.js', '/build/default~cc515e6e.js', '/build/other.js'],
-                                css: ['/build/default~cc515e6e.css']
+                                js: ['/build/runtime.js', '/build/0.js', '/build/1.js', '/build/other.js'],
+                                css: ['/build/1.css']
                             }
                         }
                     });
 
                     // make split chunks are correct in manifest
-                    webpackAssert.assertManifestKeyExists('build/vendors~cc515e6e.js');
+                    webpackAssert.assertManifestKeyExists('build/0.js');
 
                     done();
                 });
@@ -1809,19 +1843,95 @@ module.exports = {
                 });
             });
 
-            it('.mjs files are supported natively', (done) => {
-                const config = createWebpackConfig('web/build', 'dev');
-                config.addEntry('main', './js/hello_world');
-                config.setPublicPath('/build');
+            it('Make sure chunkIds do not change between builds', (done) => {
+                // https://github.com/symfony/webpack-encore/issues/461
+                const createSimilarConfig = function(includeExtraEntry) {
+                    const config = createWebpackConfig('web/build', 'production');
+                    config.addEntry('main1', './js/code_splitting');
+                    if (includeExtraEntry) {
+                        config.addEntry('main2', './js/eslint');
+                    }
+                    config.addEntry('main3', './js/no_require');
+                    config.setPublicPath('/build');
 
-                testSetup.runWebpack(config, (webpackAssert) => {
-                    // check that main.js has the correct contents
-                    webpackAssert.assertOutputFileContains(
-                        'main.js',
-                        'Hello World!'
-                    );
+                    return config;
+                };
 
-                    done();
+                const configA = createSimilarConfig(false);
+                const configB = createSimilarConfig(true);
+
+                testSetup.runWebpack(configA, () => {
+                    testSetup.runWebpack(configB, () => {
+                        const main3Contents = readOutputFileContents('main3.js', configA);
+                        const finalMain3Contents = readOutputFileContents('main3.js', configB);
+
+                        if (finalMain3Contents !== main3Contents) {
+                            throw new Error(`Contents after first compile do not match after second compile: \n\n ${main3Contents} \n\n versus \n\n ${finalMain3Contents} \n`);
+                        }
+
+                        done();
+                    });
+                });
+            });
+
+            it('Do not change contents or filenames when more modules require the same split contents', (done) => {
+                const createSimilarConfig = function(includeExtraEntry) {
+                    const config = createWebpackConfig('web/build', 'production');
+                    config.addEntry('main1', ['./js/code_splitting', 'preact']);
+                    config.addEntry('main3', ['./js/no_require', 'preact']);
+                    if (includeExtraEntry) {
+                        config.addEntry('main4', ['./js/eslint', 'preact']);
+                    }
+                    config.setPublicPath('/build');
+                    config.splitEntryChunks();
+                    config.configureSplitChunks((splitChunks) => {
+                        // will include preact, but prevent any other splitting
+                        splitChunks.minSize = 10000;
+                    });
+
+                    return config;
+                };
+
+                const getSplitVendorJsPath = function(config) {
+                    const entrypointData = getEntrypointData(config, 'main3');
+
+                    const splitFiles = entrypointData.js.filter(filename => {
+                        return filename !== '/build/runtime.js' && filename !== '/build/main3.js';
+                    });
+
+                    // sanity check
+                    if (splitFiles.length !== 1) {
+                        throw new Error(`Unexpected number (${splitFiles.length}) of split files for main3 entry`);
+                    }
+
+                    return splitFiles[0];
+                };
+
+                const configA = createSimilarConfig(false);
+                const configB = createSimilarConfig(true);
+
+                testSetup.runWebpack(configA, () => {
+                    testSetup.runWebpack(configB, () => {
+                        const vendorPath = getSplitVendorJsPath(configA);
+                        const finalVendorPath = getSplitVendorJsPath(configB);
+
+                        // make sure that the filename of the split vendor file didn't change,
+                        // even though an additional entry is now sharing its contents
+                        if (finalVendorPath !== vendorPath) {
+                            throw new Error(`Vendor filename changed! Before ${vendorPath} and after ${finalVendorPath}.`);
+                        }
+
+                        // make sure that, internally, the split chunk name did not change,
+                        // which would cause the contents of main3 to suddenly change
+                        const main3Contents = readOutputFileContents('main3.js', configA);
+                        const finalMain3Contents = readOutputFileContents('main3.js', configB);
+
+                        if (finalMain3Contents !== main3Contents) {
+                            throw new Error(`Contents after first compile do not match after second compile: \n\n ${main3Contents} \n\n versus \n\n ${finalMain3Contents} \n`);
+                        }
+
+                        done();
+                    });
                 });
             });
         });