refactor: update to new plugin system and API's #707

horo42 · 2018-01-28T01:53:19Z

The plugin stopped working with webpack 4 because of breaking changes. I hacked away at it for a couple of hours and managed to get it to build without errors.

I haven't seen a lot of activity here, so I figured I'd share my work and maybe spare someone some time.

jsf-clabot · 2018-01-28T01:53:20Z

All committers have signed the CLA.

michael-ciniawsky

@horo42 Thx

TheLarkInn · 2018-01-28T19:50:38Z

Wow! Thank you!!!

alexander-akait

Oh, wow, thanks!

sciyoshi · 2018-01-30T05:03:26Z

src/lib/helpers.js

+  const parentCount = Array.from(
+    chunk.groupsIterable,
+    (chunkGroup) => chunkGroup.getParents().length
+  ).reduce((current, sum) => current + sum);


This was giving me an error:

TypeError: Reduce of empty array with no initial value at Array.reduce (<anonymous>:null:null) at isInitialOrHasNoParents (/home/sciyoshi/workspace/web/webpack/extract-text-webpack-plugin/dist/lib/helpers.js:15:102)

Changing the line to ).reduce((current, sum) => current + sum, 0); fixed it.

Thank you! I noticed that just yesterday, along with failing integration test. I didn’t push yet because I couldn’t make it pass everything.

I’ll fix and push the obvious low hanging fruits I missed and will keep working on the tests.

Expect an update ~12 hours later.

@horo42 also can you add tests around this?

horo42 · 2018-01-30T20:58:51Z

src/index.js

+              if (chunkGroup instanceof Entrypoint) {
+                extractedChunk.addGroup(chunkGroup);
+              }
+


We add the ChunkGroups to the extracted chunk and the _children and _parents sets belong to ChunkGroup now. Please confirm that the commented lines below are unnecessary.

I can't 💯 atm 😛, but likely that this can be removed with the introduction of ChunkGroups now

Please leave everything commented for now, I will tinker with this code once the PR landed :)

horo42 · 2018-01-30T23:02:18Z

Test output: https://pastebin.com/z5FiXqk7

I'm not entirely sure where to look for these bugs. Guidance is more than welcome.
Will keep looking in my free time.

michael-ciniawsky · 2018-02-02T04:50:07Z

@horo42 I will chime in deeper into this PR in a few days, feel free to leave comments/questions as so see fit in the meantime and I will try to work through them then :)

TheLarkInn · 2018-02-07T21:43:10Z

@michael-ciniawsky @sokra @ooflorent would one of you give this a quick pass rereview? I'd like to merge this to next branch and have a -next build shipped for this if theres no blockers on it.

michael-ciniawsky · 2018-02-08T18:16:06Z

Once the requested changes by @ooflorent are addressed I will cut an alpha release with this PR so folks can start trying it out and outstanding fixes can simply follow up

michael-ciniawsky · 2018-02-08T18:33:13Z

src/index.js

+        (callback) => {
+          extractedChunks.forEach((extractedChunk) => {
+            if (extractedChunk.getNumberOfModules()) {
+              extractedChunk.sortModules((a, b) => {


sortModules is deprecated aswell, not to important though (leave it if it anything else causes failures) but if you can refactor it to use a standard Array.prototype.sort instead it would be great 👍. Maybe chunk.getModules().sort() works here

michael-ciniawsky · 2018-02-09T08:30:57Z

appveyor.yml

@@ -7,13 +7,13 @@ init:
 environment:
  matrix:
    - nodejs_version: '8'
-      webpack_version: latest
+      webpack_version: '4.0.0-beta.0'


Please don't alter this, we have a boilerplate package (webpack-defaults) which automatically updates these files

What is not working here atm ?

Sorry about that 😅 I'll revert that change.

michael-ciniawsky · 2018-02-09T08:31:59Z

src/loader.js

@@ -8,16 +8,18 @@ import SingleEntryPlugin from 'webpack/lib/SingleEntryPlugin';
 import LimitChunkCountPlugin from 'webpack/lib/optimize/LimitChunkCountPlugin';

 const NS = path.dirname(fs.realpathSync(__filename));
+const pluginName = 'ExtractTextPlugin';


const plugin = { name: 'ExtractTextPlugin' }

horo42 · 2018-02-10T22:30:05Z

src/index.js

+    for (const chunkModule of chunk.modulesIterable) {
+      let moduleSource = chunkModule.source();
+
+      if (moduleSource instanceof CachedSource) {


During my testing I noticed that async imports are CachedSource module instances caching a ReplaceSource instance, which breaks the plugin.

The issue probably lies elsewhere, this is a temporary fix.

Please add a comment for this to the code :)

horo42 · 2018-02-10T22:35:40Z

src/index.js

-                  new OrderUndefinedError(b.getOriginalModule())
-                );
-              }
-              return getOrder(a, b);


The getOrder helper function uses the underlying index2 property of modules. Unfortunately this property seems to be set by webpack after sealing and apparently sealing happens after the additionalAssets hook (where the sorting takes place) because accessing index2 yielded null every time.

Please add a comment for this to the code :)

horo42 · 2018-02-10T23:01:57Z

🎉 Tests: 1 failed, 37 passed, 38 total

The only remaining failing test is the CSS import order test case. I guess it fails because the test assumes a depth first order and webpack does it breadth first.

michael-ciniawsky · 2018-02-11T07:47:24Z

src/loader.js

+    // a child compiler so we don't spawn other child compilers from there.
+    childCompiler.hooks.thisCompilation.tap(plugin.name, (compilation) => {
+      compilation.hooks.normalModuleLoader.tap(
+        plugin.name,


You can just pass the {Object} here, since in theory plugin could contain { name: 'MyPlugin', before: 'SomePlugin', after: 'SomePlugin' }

michael-ciniawsky

One nitpick and please add comments to temporary fixes/workarounds, so we don't miss them. This is great work and I would like to merge this as soon as possible and release and alpha so this can be tested by others 🎉

TheLarkInn · 2018-02-14T00:07:28Z

@michael-ciniawsky Looks like changes have been added. Are these enough to ship alpha?

log2-hwan · 2018-02-14T06:36:33Z

src/index.js

-    }, this);
+
+    for (const chunkModule of chunk.modulesIterable) {
+      let moduleSource = chunkModule.source();


Seems this code causes runtime error when chunkModule is ConcatenatedModule instance (https://github.com/webpack/webpack/blob/master/lib/optimize/ConcatenatedModule.js#L370). ConcatenatedModule's source function requires dependencyTemplate and runtimeTemplate parameters.

Can you please share the code that causes this runtime error when compiled? If the code isn’t public, could you please try removing as much code as possible so as to still exit with a runtime error?

Thanks!

@log2-hwan Please open an issue about this with some logs if possible

) BREAKING CHANGE: requires `webpack >= v4.0.0`

michael-ciniawsky · 2018-02-14T11:46:43Z

https://github.com/webpack-contrib/extract-text-webpack-plugin/releases/tag/v4.0.0-alpha.0 🎉

) BREAKING CHANGE: requires `webpack >= v4.0.0`

horo42 added 5 commits January 28, 2018 01:12

refactor: Update to webpack4-beta.0, fix deprecations

4025fb3

refactor: keep variable names consistent

1612194

refactor: fix deprecation and eslint warnings

2fae22f

fix: use getter for parents and children

4799c0b

refactor: fix deprecations

1cda4c3

michael-ciniawsky changed the title ~~Add webpack beta0 support, fix most deprecation warnings~~ refactor: update to new plugin system and API's Jan 28, 2018

michael-ciniawsky added priority: 2 (urgent) semver: Major pr_action: Review type: Refactor status: Approved labels Jan 28, 2018

michael-ciniawsky added this to the 4.0.0 milestone Jan 28, 2018

michael-ciniawsky approved these changes Jan 28, 2018

View reviewed changes

michael-ciniawsky requested review from alexander-akait and joshwiens January 28, 2018 04:11

alexander-akait approved these changes Jan 29, 2018

View reviewed changes

sciyoshi reviewed Jan 30, 2018

View reviewed changes

horo42 added 4 commits January 30, 2018 21:24

fix: remove CommonsChunksPlugin, use optimization.splitChunks instead

6f0530d

docs: remove CommonsChunksPlugin mention from README

22aad20

fix and refactor code to make more integration tests pass

901e653

fix isInitialOrHasNoParents parent count

0f94c96

horo42 commented Jan 30, 2018

View reviewed changes

remove sortModules call to make integration tests pass

ab1efc3

andriijas mentioned this pull request Jan 31, 2018

Update to webpack 4 and integrate automatic vendor code splitting facebook/create-react-app#3878

Closed

robmcguinness mentioned this pull request Feb 4, 2018

Webpack 4 Availity/availity-workflow#140

Closed

michael-ciniawsky reviewed Feb 8, 2018

View reviewed changes

horo42 added 3 commits February 8, 2018 22:32

appveyor: set webpack version to 4.0.0-beta.0 explicitly

23c9aef

refactor: apply suggested changes from reviews

e2350c7

fix: eslint error and warnings

75a37ea

michael-ciniawsky suggested changes Feb 9, 2018

View reviewed changes

horo42 added 5 commits February 10, 2018 23:14

revert: appveyor webpack version strings

937306a

update to webpack:4.0.0-beta.1

948ad96

refactor: general code cleanup

949825f

fix: async bugfix

0d412ef

fix: add temporary fix for async imports

2b1a687

horo42 commented Feb 10, 2018

View reviewed changes

horo42 added 2 commits February 10, 2018 23:48

fix: remove continue statement

86a4a16

fix: remove module sorting, fix async import order

ed43282

michael-ciniawsky reviewed Feb 11, 2018

View reviewed changes

michael-ciniawsky approved these changes Feb 11, 2018

View reviewed changes

horo42 added 3 commits February 11, 2018 14:09

refactor: clean up async code and plugin params

ead30cd

refactor: add comments to workarounds, reintroduce sorting

66dff63

test: update snapshot

0d22337

log2-hwan reviewed Feb 14, 2018

View reviewed changes

michael-ciniawsky merged commit 9e12b8e into webpack-contrib:next Feb 14, 2018

michael-ciniawsky removed pr_action: Review priority: 2 (urgent) labels Feb 14, 2018

michael-ciniawsky pushed a commit that referenced this pull request Feb 14, 2018

refactor(ExtractTextPlugin): update to new plugin system and API's (#707

51c56c0

) BREAKING CHANGE: requires `webpack >= v4.0.0`

pi0 mentioned this pull request Feb 26, 2018

✨ Webpack 4 upgrade nuxt/nuxt#2686

Closed

joshwiens pushed a commit that referenced this pull request Apr 2, 2018

refactor(ExtractTextPlugin): update to new plugin system and API's (#707

9ad2421

) BREAKING CHANGE: requires `webpack >= v4.0.0`

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: update to new plugin system and API's #707

refactor: update to new plugin system and API's #707

horo42 commented Jan 28, 2018

jsf-clabot commented Jan 28, 2018 •

edited

Loading

michael-ciniawsky left a comment

TheLarkInn commented Jan 28, 2018

alexander-akait left a comment

sciyoshi Jan 30, 2018

horo42 Jan 30, 2018

alexander-akait Jan 30, 2018

horo42 Jan 30, 2018 •

edited

Loading

michael-ciniawsky Feb 2, 2018

michael-ciniawsky Feb 8, 2018 •

edited

Loading

horo42 commented Jan 30, 2018

michael-ciniawsky commented Feb 2, 2018 •

edited

Loading

TheLarkInn commented Feb 7, 2018

michael-ciniawsky commented Feb 8, 2018

michael-ciniawsky Feb 8, 2018 •

edited

Loading

michael-ciniawsky Feb 9, 2018

michael-ciniawsky Feb 9, 2018

horo42 Feb 10, 2018

michael-ciniawsky Feb 9, 2018

horo42 Feb 10, 2018 •

edited

Loading

michael-ciniawsky Feb 11, 2018

horo42 Feb 10, 2018 •

edited

Loading

michael-ciniawsky Feb 11, 2018

horo42 commented Feb 10, 2018

michael-ciniawsky Feb 11, 2018

michael-ciniawsky left a comment •

edited

Loading

TheLarkInn commented Feb 14, 2018

log2-hwan Feb 14, 2018

horo42 Feb 14, 2018

michael-ciniawsky Feb 14, 2018

michael-ciniawsky commented Feb 14, 2018

refactor: update to new plugin system and API's #707

refactor: update to new plugin system and API's #707

Conversation

horo42 commented Jan 28, 2018

jsf-clabot commented Jan 28, 2018 • edited Loading

michael-ciniawsky left a comment

Choose a reason for hiding this comment

TheLarkInn commented Jan 28, 2018

alexander-akait left a 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

horo42 Jan 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-ciniawsky Feb 8, 2018 • edited Loading

Choose a reason for hiding this comment

horo42 commented Jan 30, 2018

michael-ciniawsky commented Feb 2, 2018 • edited Loading

TheLarkInn commented Feb 7, 2018

michael-ciniawsky commented Feb 8, 2018

michael-ciniawsky Feb 8, 2018 • edited Loading

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

horo42 Feb 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

horo42 Feb 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

horo42 commented Feb 10, 2018

Choose a reason for hiding this comment

michael-ciniawsky left a comment • edited Loading

Choose a reason for hiding this comment

TheLarkInn commented Feb 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-ciniawsky commented Feb 14, 2018

jsf-clabot commented Jan 28, 2018 •

edited

Loading

horo42 Jan 30, 2018 •

edited

Loading

michael-ciniawsky Feb 8, 2018 •

edited

Loading

michael-ciniawsky commented Feb 2, 2018 •

edited

Loading

michael-ciniawsky Feb 8, 2018 •

edited

Loading

horo42 Feb 10, 2018 •

edited

Loading

horo42 Feb 10, 2018 •

edited

Loading

michael-ciniawsky left a comment •

edited

Loading