From be3cb13a1d3cff835961f4e4f3299ba70e4ad207 Mon Sep 17 00:00:00 2001 From: kosciolek <45483493+kosciolek@users.noreply.github.com> Date: Mon, 8 Apr 2024 23:14:41 +0200 Subject: [PATCH 1/2] fix: bad module order and missing modules when optimizing side effect free modules --- .../index-dependency-dependency.css | 3 ++ .../index-dependency-dependency.js | 3 ++ .../index-dependency.css | 3 ++ .../index-dependency.js | 9 +++++ .../order-of-side-effect-modules/index.css | 3 ++ .../order-of-side-effect-modules/index.js | 35 +++++++++++++++++++ .../order-of-side-effect-modules/package.json | 3 ++ .../webpack.config.js | 11 ++++++ 8 files changed, 70 insertions(+) create mode 100644 test/configCases/optimization/order-of-side-effect-modules/index-dependency-dependency.css create mode 100644 test/configCases/optimization/order-of-side-effect-modules/index-dependency-dependency.js create mode 100644 test/configCases/optimization/order-of-side-effect-modules/index-dependency.css create mode 100644 test/configCases/optimization/order-of-side-effect-modules/index-dependency.js create mode 100644 test/configCases/optimization/order-of-side-effect-modules/index.css create mode 100644 test/configCases/optimization/order-of-side-effect-modules/index.js create mode 100644 test/configCases/optimization/order-of-side-effect-modules/package.json create mode 100644 test/configCases/optimization/order-of-side-effect-modules/webpack.config.js diff --git a/test/configCases/optimization/order-of-side-effect-modules/index-dependency-dependency.css b/test/configCases/optimization/order-of-side-effect-modules/index-dependency-dependency.css new file mode 100644 index 00000000000..9a143ab169a --- /dev/null +++ b/test/configCases/optimization/order-of-side-effect-modules/index-dependency-dependency.css @@ -0,0 +1,3 @@ +.index-dependency-dependency { + background-color: red; +} diff --git a/test/configCases/optimization/order-of-side-effect-modules/index-dependency-dependency.js b/test/configCases/optimization/order-of-side-effect-modules/index-dependency-dependency.js new file mode 100644 index 00000000000..761eec7c020 --- /dev/null +++ b/test/configCases/optimization/order-of-side-effect-modules/index-dependency-dependency.js @@ -0,0 +1,3 @@ +import "./index-dependency-dependency.css"; + +export const indexDependencyDependency = () => {}; diff --git a/test/configCases/optimization/order-of-side-effect-modules/index-dependency.css b/test/configCases/optimization/order-of-side-effect-modules/index-dependency.css new file mode 100644 index 00000000000..0908338722b --- /dev/null +++ b/test/configCases/optimization/order-of-side-effect-modules/index-dependency.css @@ -0,0 +1,3 @@ +.index-dependency { + background-color: aqua; +} diff --git a/test/configCases/optimization/order-of-side-effect-modules/index-dependency.js b/test/configCases/optimization/order-of-side-effect-modules/index-dependency.js new file mode 100644 index 00000000000..7dbd182ab25 --- /dev/null +++ b/test/configCases/optimization/order-of-side-effect-modules/index-dependency.js @@ -0,0 +1,9 @@ +// this would work +// import { indexDependencyDependency as A } from "./index-dependency-dependency"; +// export const indexDependencyDependency = A + +export { indexDependencyDependency } from "./index-dependency-dependency"; + +import "./index-dependency.css"; + +export const indexDependency = () => {}; diff --git a/test/configCases/optimization/order-of-side-effect-modules/index.css b/test/configCases/optimization/order-of-side-effect-modules/index.css new file mode 100644 index 00000000000..64f9c3b658a --- /dev/null +++ b/test/configCases/optimization/order-of-side-effect-modules/index.css @@ -0,0 +1,3 @@ +.index { + background-color: black; +} diff --git a/test/configCases/optimization/order-of-side-effect-modules/index.js b/test/configCases/optimization/order-of-side-effect-modules/index.js new file mode 100644 index 00000000000..5b7f0fdc1b0 --- /dev/null +++ b/test/configCases/optimization/order-of-side-effect-modules/index.js @@ -0,0 +1,35 @@ +import { indexDependencyDependency } from "./index-dependency"; +import "./index.css"; + +indexDependencyDependency(); + +it("correct module order with \"sideEffects\": [\"*.css\"]", function (done) { + const postOrder = __STATS__.modules + .filter(mod => mod.name.endsWith(".css")) + .map(mod => ({ module: mod.name, postOrderIndex: mod.postOrderIndex })); + + console.log(postOrder); + + // this is the PROPER order. + // we get it if we don't use use `"sideEffects": ["*.css"]` in package.json + // it should be the same with the setting on, but it's not, which is a bug + // since mini-css-extract-plugin loads .css modules in postOrder order + // our css classes would be index-dependency-dependency.css (least important) -> index-dependency.css -> index.css + expect(postOrder).toStrictEqual([ + { module: "./index.css", postOrderIndex: 5 }, + { module: "./index-dependency.css", postOrderIndex: 3 }, + { module: "./index-dependency-dependency.css", postOrderIndex: 1 } + ]); + + + // this is the actual order with `"sideEffects": ["*.css"]` + const actualBuggyOrder = [ + { module: './index.css', postOrderIndex: 1 }, + // bug: index-dependency-depednency.css will override index.css, even though in reality it was imported first + { module: './index-dependency-dependency.css', postOrderIndex: 2 }, + // another bug: index-dependency.css is missing from the bundle entirely, even though it should be loaded + { module: './index-dependency.css', postOrderIndex: null } + ] + + done(); +}); diff --git a/test/configCases/optimization/order-of-side-effect-modules/package.json b/test/configCases/optimization/order-of-side-effect-modules/package.json new file mode 100644 index 00000000000..1e84294f864 --- /dev/null +++ b/test/configCases/optimization/order-of-side-effect-modules/package.json @@ -0,0 +1,3 @@ +{ + "sideEffects": ["*.css"] +} diff --git a/test/configCases/optimization/order-of-side-effect-modules/webpack.config.js b/test/configCases/optimization/order-of-side-effect-modules/webpack.config.js new file mode 100644 index 00000000000..9027d7b7687 --- /dev/null +++ b/test/configCases/optimization/order-of-side-effect-modules/webpack.config.js @@ -0,0 +1,11 @@ +/** @type {import("../../../../").Configuration} */ +module.exports = { + module: { + rules: [ + { + test: /\.css$/, + use: ["css-loader"] + } + ] + } +}; From a15828394e409de9fe41153bca07be3f87d14f26 Mon Sep 17 00:00:00 2001 From: kosciolek <45483493+kosciolek@users.noreply.github.com> Date: Wed, 10 Apr 2024 14:15:01 +0200 Subject: [PATCH 2/2] commenting this code makes the failing test case pass --- lib/optimize/SideEffectsFlagPlugin.js | 45 ++++++++++++++------------- 1 file changed, 23 insertions(+), 22 deletions(-) diff --git a/lib/optimize/SideEffectsFlagPlugin.js b/lib/optimize/SideEffectsFlagPlugin.js index ab59c11f532..be10d291f21 100644 --- a/lib/optimize/SideEffectsFlagPlugin.js +++ b/lib/optimize/SideEffectsFlagPlugin.js @@ -281,6 +281,7 @@ class SideEffectsFlagPlugin { if (optimizedModules.has(module)) return; optimizedModules.add(module); if (module.getSideEffectsConnectionState(moduleGraph) === false) { + // eslint-disable-next-line no-unused-vars const exportsInfo = moduleGraph.getExportsInfo(module); for (const connection of moduleGraph.getIncomingConnections( module @@ -327,29 +328,29 @@ class SideEffectsFlagPlugin { continue; } // TODO improve for nested imports - const ids = dep.getIds(moduleGraph); - if (ids.length > 0) { - const exportInfo = exportsInfo.getExportInfo(ids[0]); - const target = exportInfo.getTarget( - moduleGraph, - ({ module }) => - module.getSideEffectsConnectionState(moduleGraph) === - false - ); - if (!target) continue; + // const ids = dep.getIds(moduleGraph); + // if (ids.length > 0) { + // const exportInfo = exportsInfo.getExportInfo(ids[0]); + // const target = exportInfo.getTarget( + // moduleGraph, + // ({ module }) => + // module.getSideEffectsConnectionState(moduleGraph) === + // false + // ); + // if (!target) continue; - moduleGraph.updateModule(dep, target.module); - moduleGraph.addExplanation( - dep, - "(skipped side-effect-free modules)" - ); - dep.setIds( - moduleGraph, - target.export - ? [...target.export, ...ids.slice(1)] - : ids.slice(1) - ); - } + // moduleGraph.updateModule(dep, target.module); + // moduleGraph.addExplanation( + // dep, + // "(skipped side-effect-free modules)" + // ); + // dep.setIds( + // moduleGraph, + // target.export + // ? [...target.export, ...ids.slice(1)] + // : ids.slice(1) + // ); + // } } } }