fix: split async CSS correctly #546

BowlingX · 2017-06-17T19:27:30Z

This is a work-in-progress pull-request to address issue #120.
I added a test case to demonstrate the issue, use-case and my expectations.

…ck-contrib#483) This is recommended after webpack/webpack#4704

jsf-clabot · 2017-06-17T19:27:31Z

All committers have signed the CLA.

BowlingX · 2017-06-17T20:41:51Z

I adjusted the extraction logic, the result is now as expected, however another test (splitted-multiple-entries) fails now. I'm not sure if I'm heading in the right direction or if we may should make this configurable.

BowlingX · 2017-06-17T22:49:27Z

ok, ignore the last commit, does not make any sense and is not working at all.

codecov · 2017-06-18T12:26:43Z

Codecov Report

❗ No coverage uploaded for pull request base (master@b254e96). Click here to learn what that means.
The diff coverage is 96.87%.

@@            Coverage Diff            @@
##             master     #546   +/-   ##
=========================================
  Coverage          ?   88.37%           
=========================================
  Files             ?        7           
  Lines             ?      327           
  Branches          ?       71           
=========================================
  Hits              ?      289           
  Misses            ?       36           
  Partials          ?        2

Impacted Files	Coverage Δ
src/lib/helpers.js	`90.24% <100%> (ø)`
src/index.js	`91.13% <96.66%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b254e96...edd39a1. Read the comment docs.

BowlingX · 2017-06-18T22:53:18Z

I think I finally found the correct way to do it. It was a little hard to get my head around it without much docs about the webpack internals, but I think it's a proper solution.

I will now actually keep the fallback loader in place for the async bundles and only rebuild it without replacing it in the async chunks. That means all css is correctly async loaded and only the css for the initial chunk is written down (if allChunks: false).

The initial chunks is freed from all dependencies and replaced with the generated module (that may contain the css-module informations).

- merged with webpack-contrib/extract-text-webpack-plugin#546 - readded async css file reload - cleanup

michael-ciniawsky · 2017-06-20T15:23:29Z

Fixes #120

michael-ciniawsky · 2017-06-20T15:26:11Z

@BowlingX WoW..😧 😛 . I'm not 💯 skilled to answer if this is suitable or not, but it looks very good

bebraw

It's too complex for me to review properly. Better get Tobias on this.

alexander-akait · 2017-06-20T15:28:31Z

@BowlingX please squash all commits in one for easy review, thanks

michael-ciniawsky · 2017-06-20T15:29:52Z

An additional 'problem' is the webpack 3 release, which requires a breaking change here #540 #543 (#542), so we need to fix this first (as quickly as possible). So maybe you need to rework a few parts because of this in any case 🙏

commit e2631af Author: David Heidrich <me@bowlingx.com> Date: Mon Jun 19 19:08:13 2017 +0200 removed dead code, cleanup commit d8943d7 Author: David Heidrich <me@bowlingx.com> Date: Mon Jun 19 15:48:51 2017 +0200 restored console log commit 0d7c94a Author: David Heidrich <me@bowlingx.com> Date: Mon Jun 19 00:30:34 2017 +0200 fixed async loading (keep original style loader in async chunks) commit 950d7ef Author: David Heidrich <me@bowlingx.com> Date: Sun Jun 18 15:52:41 2017 +0200 fixed exporting module informations commit 8b97ee5 Author: David Heidrich <me@bowlingx.com> Date: Sun Jun 18 14:27:51 2017 +0200 fixed line length commit 6898aa4 Author: David Heidrich <me@bowlingx.com> Date: Sun Jun 18 14:26:34 2017 +0200 respect all chunks commit bbc93ab Author: David Heidrich <me@bowlingx.com> Date: Sun Jun 18 12:33:09 2017 +0200 adjusted test to be env agnostic commit 3eba627 Author: David Heidrich <me@bowlingx.com> Date: Sun Jun 18 12:25:16 2017 +0200 adjusted tests to use css-loader, added tests for allChunks, remove module from initial chunk commit 09ac8cb Author: David Heidrich <me@bowlingx.com> Date: Sat Jun 17 22:36:09 2017 +0200 adjusted tests, remove module from base if extracted commit 1791796 Author: David Heidrich <me@bowlingx.com> Date: Sat Jun 17 21:23:58 2017 +0200 added testcase for issue webpack-contrib#120

joshwiens

Should we be targeting master with this or the feature/webpack3. This PR is wasted putting it in the current version range as it's only applicable to Webpack v2

joshwiens · 2017-06-20T20:54:39Z

We can always back port it to the 2.x version range after the fact.

BowlingX · 2017-07-26T14:08:13Z

Seems like vue is handling async css loading on it's own. I think this get's in the way how extrac-text-webpack-plugin and my changes are doing this right now. So I would not expect that to be working together.

nkonev · 2017-07-26T14:27:44Z

@BowlingX
Thank you for response

@michael-ciniawsky @sokra @d3viant0ne @BowlingX
I also found that I can fix it by disable webpack's cache (introduced in 3.0.0)

Unfortunately there isn't activity in https://github.com/vuejs/vue-style-loader, ok, I'll try to create issue there (vue-style-loader repo).

@michael-ciniawsky @sokra @d3viant0ne
But before it I want to ensure that trouble is exactly in vue-style-loader =) (Disabling Webpack cache solves it)

BowlingX · 2017-08-04T08:48:46Z

What is the status for this pull-request? Is there any more work needed?

BowlingX · 2017-08-15T18:43:36Z

I was able to reproduce the issue that @nikit-cpp had. Something is not right here, I will investigate

nkonev · 2017-08-16T17:48:45Z

@BowlingX I can check if need

michael-ciniawsky · 2017-08-16T17:52:00Z

The approach looks fine so far (from what i can tell :)) but as mentioned before I can't fully approve the changes. @sokra needs to review this PR to ensure this doesn't break the plugin

alexander-akait · 2017-09-13T12:50:34Z

src/index.js

@@ -26,7 +27,7 @@ class ExtractTextPlugin {
    if (isString(options)) {
      options = { filename: options };
    } else {
-      validateOptions(path.resolve(__dirname, '../schema/plugin.json'), options, 'Extract Text Plugin');
+      validateOptions(path.resolve(__dirname, './schema/plugin.json'), options, 'Extract Text Plugin');


./schema is not resolved correctly when I build the branch from source. Not sure how the build process is, but without changing the path it does not compile.

This should be reverted and the schema folder moved back to where it was.

Ideally move schema/Plugin.json && schema/loader.json from /schema to /src/Plugin.json && /src/loader.json and add --copy-files to the build script in package.json please.

- schema src ||– index.js ||– Plugin.json ||- loader.js ||- loader.json | |-package.json

alexander-akait · 2017-09-13T12:50:53Z

src/index.js

@@ -40,6 +41,17 @@ class ExtractTextPlugin {
    return { loader: require.resolve('./loader'), options };
  }

+  static cloneModule(module) {


Can we move this in https://github.com/webpack-contrib/extract-text-webpack-plugin/blob/master/src/lib/helpers.js

alexander-akait · 2017-09-13T12:51:08Z

src/index.js

@@ -145,25 +159,38 @@ class ExtractTextPlugin {
          const extractedChunk = extractedChunks[chunks.indexOf(chunk)];
          const shouldExtract = !!(options.allChunks || isInitialOrHasNoParents(chunk));
          chunk.sortModules();
-          async.forEach(chunk.mapModules(c => c), (module, callback) => { // eslint-disable-line no-shadow
+          async.forEach(chunk.mapModules((c) => { return c; }), (module, callback) => { // eslint-disable-line no-shadow, arrow-body-style


can be changed to c => c, properly a missed merge

oviava · 2017-10-19T11:49:51Z

does this have any more traction ? run some local tests with the forked branch and seems ok

FlorianH · 2017-11-14T11:12:13Z

+1

danez

We had to use this here at my company because of the issue this is solving. Works fine so far.
The PR lgtm only the schema folder should be moved back to where it was imho and the stuff that was mentioned by others.

danez · 2017-11-29T10:03:46Z

src/index.js

@@ -26,7 +27,7 @@ class ExtractTextPlugin {
    if (isString(options)) {
      options = { filename: options };
    } else {
-      validateOptions(path.resolve(__dirname, '../schema/plugin.json'), options, 'Extract Text Plugin');
+      validateOptions(path.resolve(__dirname, './schema/plugin.json'), options, 'Extract Text Plugin');


This should be reverted and the schema folder moved back to where it was.

danez · 2017-11-29T10:04:12Z

src/index.js

@@ -88,7 +100,7 @@ class ExtractTextPlugin {
    if (Array.isArray(options) || isString(options) || typeof options.options === 'object' || typeof options.query === 'object') {
      options = { use: options };
    } else {
-      validateOptions(path.resolve(__dirname, '../schema/loader.json'), options, 'Extract Text Plugin (Loader)');
+      validateOptions(path.resolve(__dirname, './schema/loader.json'), options, 'Extract Text Plugin (Loader)');


Should also be reverted.

If I run the tests with ../schema I get:

ENOENT: no such file or directory, open '/home/david/Projekte/extract-text-webpack-plugin/schema/loader.json'

Because you moved the schema folder into the src folder, but @michael-ciniawsky wants to have them in a different location anyway, see other comment.

BowlingX · 2017-11-29T10:13:06Z

I will do the suggested changes and solve the conflicts today.

# Conflicts: # src/index.js

michael-ciniawsky

Only whitespace nits. Could you also move the various eslint-disable-line comments to the top please ?

/* eslint-disable
  ...,
  ...,
*/

michael-ciniawsky · 2017-11-29T22:59:20Z

src/index.js

@@ -26,7 +27,7 @@ class ExtractTextPlugin {
    if (isString(options)) {
      options = { filename: options };
    } else {
-      validateOptions(path.resolve(__dirname, '../schema/plugin.json'), options, 'Extract Text Plugin');
+      validateOptions(path.resolve(__dirname, './schema/plugin.json'), options, 'Extract Text Plugin');


Ideally move schema/Plugin.json && schema/loader.json from /schema to /src/Plugin.json && /src/loader.json and add --copy-files to the build script in package.json please.

- schema src ||– index.js ||– Plugin.json ||- loader.js ||- loader.json | |-package.json

michael-ciniawsky · 2017-11-29T22:59:44Z

src/index.js

@@ -126,8 +127,10 @@ class ExtractTextPlugin {
      const filename = this.filename;
      const id = this.id;
      let extractedChunks;
+      let toRemoveModules;


michael-ciniawsky · 2017-11-29T22:59:51Z

src/index.js

      compilation.plugin('optimize-tree', (chunks, modules, callback) => {
        extractedChunks = chunks.map(() => new Chunk());
+        toRemoveModules = [];


michael-ciniawsky · 2017-11-29T23:00:08Z

src/index.js

            let meta = module[NS];
            if (meta && (!meta.options.id || meta.options.id === id)) {
              const wasExtracted = Array.isArray(meta.content);
              // A stricter `shouldExtract !== wasExtracted` check to guard against cases where a previously extracted
              // module would be extracted twice. Happens when a module is a dependency of an initial and a non-initial
              // chunk. See issue #604
              if (shouldExtract && !wasExtracted) {
-                module[`${NS}/extract`] = shouldExtract; // eslint-disable-line no-path-concat
-                compilation.rebuildModule(module, (err) => {
+                const newModule = cloneModule(module);


michael-ciniawsky · 2017-11-29T23:00:20Z

src/index.js

-                module[`${NS}/extract`] = shouldExtract; // eslint-disable-line no-path-concat
-                compilation.rebuildModule(module, (err) => {
+                const newModule = cloneModule(module);
+                newModule[`${NS}/extract`] = shouldExtract; // eslint-disable-line no-path-concat


michael-ciniawsky · 2017-11-29T23:02:43Z

src/index.js

+          const data = toRemoveModules[module.identifier()];
+          if (data) {
+            const oldModuleId = module.id;
+            const newModule = cloneModule(module);


michael-ciniawsky · 2017-11-29T23:02:57Z

src/index.js

+            const oldModuleId = module.id;
+            const newModule = cloneModule(module);
+            newModule.id = oldModuleId;
+            newModule._source = data.module._source; // eslint-disable-line no-underscore-dangle


michael-ciniawsky · 2017-11-29T23:03:09Z

src/index.js

+            newModule.id = oldModuleId;
+            newModule._source = data.module._source; // eslint-disable-line no-underscore-dangle
+            data.chunks.forEach((chunk) => {
+              chunk.removeModule(data.moduleToRemove);


michael-ciniawsky · 2017-11-29T23:03:16Z

src/index.js

+            newModule._source = data.module._source; // eslint-disable-line no-underscore-dangle
+            data.chunks.forEach((chunk) => {
+              chunk.removeModule(data.moduleToRemove);
+              const deps = data.moduleToRemove.dependencies;


michael-ciniawsky · 2017-11-29T23:03:25Z

src/index.js

+                if (d.module && d.module.loaders.length > 0) {
+                  chunk.removeModule(d.module);
+                }
+              });


alexander-akait

Looks good see @michael-ciniawsky note

BowlingX · 2018-03-18T20:08:28Z

I made the requested adjustments, Is there anything more needed here? The request is quite old, I'm wondering how to proceed here.

# Conflicts: # src/index.js

alexander-akait · 2018-04-05T09:42:06Z

@BowlingX Looks like we can close this PR, don't use ETWP for extract css, for this please use https://github.com/webpack-contrib/mini-css-extract-plugin. ETWP only for extract assets.

BowlingX · 2018-04-05T09:53:41Z

@evilebottnawi I guess yes. I tested the new plugin and it works well for me. Also with the last changes (#604) my approach does not work anymore and I would have to refactor a bit. But properly it's unecessary because of the new plugin.

sokra and others added 2 commits June 10, 2017 19:34

refactor: pass a unique compiler name to get child compilation (webpa…

77daf83

…ck-contrib#483) This is recommended after webpack/webpack#4704

chore(package): Update peerDeps & webpack devDep

92e4349

BowlingX mentioned this pull request Jun 17, 2017

Performance issues faceyspacey/extract-css-chunks-webpack-plugin#14

Closed

BowlingX added a commit to BowlingX/extract-css-chunks-webpack-plugin that referenced this pull request Jun 19, 2017

- merged with latest extract-text-webpack-plugin

f392b88

- merged with webpack-contrib/extract-text-webpack-plugin#546 - readded async css file reload - cleanup

michael-ciniawsky changed the title ~~Testcase for #120, splitting async css~~ fix: split async CSS correctly Jun 20, 2017

michael-ciniawsky requested review from bebraw, alexander-akait and joshwiens June 20, 2017 15:24

michael-ciniawsky added priority: 3 (required) pr_action: Review status: Needs triage type: Bug labels Jun 20, 2017

michael-ciniawsky added this to the 2.2.0 milestone Jun 20, 2017

bebraw reviewed Jun 20, 2017

View reviewed changes

michael-ciniawsky requested a review from sokra June 20, 2017 15:40

BowlingX force-pushed the master branch from e2631af to cdfccb4 Compare June 20, 2017 20:18

joshwiens suggested changes Jun 20, 2017

View reviewed changes

joshwiens added pr_action: Rework and removed pr_action: Review labels Jun 20, 2017

alexander-akait suggested changes Sep 13, 2017

View reviewed changes

danez approved these changes Nov 29, 2017

View reviewed changes

BowlingX added 4 commits November 29, 2017 22:28

Merge remote-tracking branch 'base/master'

163191a

# Conflicts: # src/index.js

adjusted tests

2ba04eb

adjusted tests

3ef2916

fixed code style, moved clone to helpers

8403a27

michael-ciniawsky approved these changes Nov 29, 2017

View reviewed changes

michael-ciniawsky requested review from alexander-akait and joshwiens November 29, 2017 23:07

michael-ciniawsky added status: Pending severity: 3 (broken) and removed status: Needs triage labels Nov 29, 2017

michael-ciniawsky added this to the 3.1.0 milestone Nov 29, 2017

alexander-akait approved these changes Nov 30, 2017

View reviewed changes

BowlingX added 2 commits March 18, 2018 21:01

code style fixes

3aed4df

moved schema files to src, adjusted build.

f1a8ce2

Merge remote-tracking branch 'upstream/master'

edd39a1

# Conflicts: # src/index.js

BowlingX closed this Apr 5, 2018

fix: split async CSS correctly #546

fix: split async CSS correctly #546

Conversation

BowlingX commented Jun 17, 2017

jsf-clabot commented Jun 17, 2017 • edited

BowlingX commented Jun 17, 2017

BowlingX commented Jun 17, 2017

codecov bot commented Jun 18, 2017 • edited

Codecov Report

BowlingX commented Jun 18, 2017

michael-ciniawsky commented Jun 20, 2017

michael-ciniawsky commented Jun 20, 2017

bebraw left a comment

Choose a reason for hiding this comment

alexander-akait commented Jun 20, 2017

michael-ciniawsky commented Jun 20, 2017

joshwiens left a comment

Choose a reason for hiding this comment

joshwiens commented Jun 20, 2017

BowlingX commented Jul 26, 2017

nkonev commented Jul 26, 2017 • edited

BowlingX commented Aug 4, 2017

BowlingX commented Aug 15, 2017

nkonev commented Aug 16, 2017

michael-ciniawsky commented Aug 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oviava commented Oct 19, 2017

FlorianH commented Nov 14, 2017

danez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BowlingX Nov 29, 2017 • edited

Choose a reason for hiding this comment

danez Nov 30, 2017 • edited

Choose a reason for hiding this comment

BowlingX commented Nov 29, 2017

michael-ciniawsky left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-akait left a comment

Choose a reason for hiding this comment

BowlingX commented Mar 18, 2018 • edited

alexander-akait commented Apr 5, 2018

BowlingX commented Apr 5, 2018

jsf-clabot commented Jun 17, 2017 •

edited

codecov bot commented Jun 18, 2017 •

edited

nkonev commented Jul 26, 2017 •

edited

BowlingX Nov 29, 2017 •

edited

danez Nov 30, 2017 •

edited

michael-ciniawsky left a comment •

edited

BowlingX commented Mar 18, 2018 •

edited