Use webpack-merge 2.0, fix react hot loader #59

eXon · 2016-12-29T01:41:15Z

All I've done is add unit test for reactHot and update webpack-merge to 2.0.0.

It's a big jump in the version, but it ensures merging the loaders is done correctly for react-hot. However, it looks like the loaders order is now changed a little bit. Not a big deal, but worth testing enough before pushing this.

andywer

I think one change is superfluous and one must be changed, but otherwise fine :)

andywer · 2016-12-29T09:36:40Z

packages/webpack-common/src/extractText.js

@@ -18,7 +18,7 @@ export {
 */
 function getLoaderConfigByType (context, webpackConfig, fileType) {
  const loaderConfig = webpackConfig.module.loaders.find(
-    (loader) => loader.test === context.fileType(fileType)
+    (loader) => regexEqual(loader.test, context.fileType(fileType))


I don't think we have to do a real regex comparison. context.fileType(fileType) should return the same regex reference throughout the whole configuration process.

The only scenario I can think of where it makes a difference: If there are two loader configs matching two file types which have semantically equal regexs to match. But I think we should not even match those as well 😉

Maybe we should add a comment here making that a little clearer...

It is unfortunally not the case anymore when I updated webpack-merge to 2.0.0. A test was failing because it couldn't find the correct loader for text/css.

Hmm. I propose we first investigate why this is happening. Looks like an easy fix to deep-compare the regexes, but that might be a minor bug in webpack-merge. If so we should not try to fix it afterwards in webpack-blocks, but fix it in webpack-merge.

I think it is because of the deep clone webpack-merge is doing behind the scene. In a strict functional way, it is expected that the Regex instance might be a different one with the same value. I'm not sure it is bad to use a more reliable way to compare regexes. I don't think we can expect two instances with the same value on different loaders. I don't think regex are seen as primitive types in javascript.

Is it really doing a deep clone of the regex (new regex with same value)? I wouldn't expect that... (Or: If it really works this way, maybe this is this the bug 😉)

andywer · 2016-12-29T09:38:05Z

packages/webpack2/__tests__/integration.test.js

@@ -23,37 +23,37 @@ test('complete webpack config creation', (t) => {
  ])

  t.is(webpackConfig.module.loaders.length, 8)
-  t.deepEqual(webpackConfig.module.loaders[0], {


Ahh, if the order has changed here, we should re-order the assertions as well. Having the indexes in the order 7, 6, 0, 1, 2 really is confusing.

andywer · 2016-12-29T09:40:07Z

You have some good points here 🙂

Just need to adjust it a little bit.

eXon

Fixed the loaders order. However, for the regex a test was failing because of that. Regex are kinda tricky and if the merge is 100% functional, it probably copy it now.

eXon · 2016-12-29T15:23:20Z

packages/webpack-common/src/extractText.js

@@ -18,7 +18,7 @@ export {
 */
 function getLoaderConfigByType (context, webpackConfig, fileType) {
  const loaderConfig = webpackConfig.module.loaders.find(
-    (loader) => loader.test === context.fileType(fileType)
+    (loader) => regexEqual(loader.test, context.fileType(fileType))


It is unfortunally not the case anymore when I updated webpack-merge to 2.0.0. A test was failing because it couldn't find the correct loader for text/css.

andywer · 2016-12-29T16:46:39Z

Guess we have to look into that slight comparison issue a little more (see comment in code).

I cannot say when I will be able to have a deeper look into it myself, though. I am at Munich airport between two connecting flights right now and visiting my girlfriend's family for the next few days... But maybe I manage to find some time :)

andywer · 2016-12-29T16:49:23Z

The bright side is, though: The integration tests did their job perfectly fine 👍😄

eXon · 2017-01-07T03:02:47Z

The problem when comparing with the regex instance instead of the value is it is order specific. Meaning if another block add a loader and then your block adds a loader with the same regex, you don't know it will not use your instance but the existing one.

I honestly believe it's better to go for comparing the regex value. That way, it is more functional :)

andywer · 2017-01-07T16:18:52Z

@eXon Let's discuss this :)

Meaning if another block add a loader and then your block adds a loader with the same regex, you don't know it will not use your instance but the existing one.

If two loaders are going to match the same file extensions then they use the same context.fileType('foo/bar') returning the same regex instance, since that is what the context.fileType is for :) That's how we ensure referential integrity.

The problem about comparing the regex' values is not only that it's slower (I really don't care for this particular use case), but that regex' are bitches that are really hard to compare. If we don't assume referential integrity anymore and always deep-compare the regex' values we encounter a new problem: Regex synonyms. Consider /\.jsx?/ vs /\.js|\.jsx/. Different values, but matching the same extensions.

Referential integrity makes comparing trivial and helps me sleep well at night, without having to fear people open issues, because a comparison does not work for their crazy edge-case 🙈😅

eXon · 2017-01-07T17:53:26Z

@andywer good news! looks like it's not a problem anymore with the last version of webpack-merge 👍

eXon · 2017-01-08T17:43:57Z

Looks like it's still not working. My node_modules folder was not up to date. I'll continue searching why the regex instance is not the same.

andywer · 2017-01-11T13:57:16Z

I was just debugging #76 and tried updating webpack-merge as well.

I found that the last webpack-merge version leaving the regular expressions intact was ~~1.0.0~~ 1.0.1

andywer · 2017-01-11T14:10:30Z

I stand corrected: webpack-merge v1.0.2 broke it and I even found out why:

They introduced a breaking change and just published as a hot fix version. This commit introduces lodash.cloneDeep to deep-clone any object it sees.

What's happening in webpack-merge (see the commit): _.isPlainObject(/regex/) would be false, but _.isPlainObject({ test: /regex/ }) will be true and I guess the following _.deepClone({ test: /regex/ }) will also deep-clone the regex... :-/

eXon · 2017-01-11T14:27:32Z

When they are comparing the loader test regex, webpack-merge is using String(regex).

Maybe it's good enough for us too?

andywer · 2017-01-11T14:30:29Z

@eXon Yeah, I give in. Let's do it like that as well, but let's add a short comment explaining that we do it because of webpack-merge deep-cloning stuff 😉

andywer · 2017-01-11T15:04:54Z

Created #77, since this was your hot reloading PR. We should start to take more care not to throw different independent features into the same PR. Otherwise we will have "spaghetti PRs" soon :)

eXon · 2017-01-11T15:25:54Z

Well the fix for the hot-reload bug was to upgrade webpack-merge but it will do the job. Awesome thanks 👍

andywer · 2017-01-11T15:48:07Z

@eXon I wasn't even aware of that anymore. So can we close this PR then? Might be good if you check out the latest master and confirm that it works.

eXon · 2017-01-13T13:20:44Z

Your PR diff is almost identical so I'll close this. Thanks for the fix!

Fix react-hot loader + update webpack-merge version

144fa05

andywer requested changes Dec 29, 2016

View reviewed changes

Reorganize loaders assertions

cfaccfb

eXon commented Dec 29, 2016

View reviewed changes

andywer changed the title ~~FIx react hot reload~~ Use webpack-merge 2.0, fix react hot loader Dec 30, 2016

andywer mentioned this pull request Dec 30, 2016

Update webpack-merge dependency to v2.0.0 #61

Closed

Merged with master

32cee0d

Change back regex reference equality for extract-text

a039caf

eXon mentioned this pull request Jan 11, 2017

Unexpected result of entry points merge #76

Closed

andywer mentioned this pull request Jan 11, 2017

Upgrade to webpack-merge 2.x #77

Merged

eXon closed this Jan 13, 2017

eXon deleted the bug/hot-reload branch January 13, 2017 13:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use webpack-merge 2.0, fix react hot loader #59

Use webpack-merge 2.0, fix react hot loader #59

eXon commented Dec 29, 2016

andywer left a comment

andywer Dec 29, 2016

andywer Dec 29, 2016

eXon Dec 29, 2016

andywer Dec 29, 2016

eXon Dec 30, 2016

andywer Dec 30, 2016

andywer Dec 29, 2016

andywer commented Dec 29, 2016

eXon left a comment

eXon Dec 29, 2016

andywer commented Dec 29, 2016

andywer commented Dec 29, 2016

eXon commented Jan 7, 2017 •

edited

Loading

andywer commented Jan 7, 2017

eXon commented Jan 7, 2017

eXon commented Jan 8, 2017

andywer commented Jan 11, 2017 •

edited

Loading

andywer commented Jan 11, 2017 •

edited

Loading

eXon commented Jan 11, 2017

andywer commented Jan 11, 2017

andywer commented Jan 11, 2017

eXon commented Jan 11, 2017

andywer commented Jan 11, 2017

eXon commented Jan 13, 2017

Use webpack-merge 2.0, fix react hot loader #59

Use webpack-merge 2.0, fix react hot loader #59

Conversation

eXon commented Dec 29, 2016

andywer 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

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

andywer commented Dec 29, 2016

eXon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andywer commented Dec 29, 2016

andywer commented Dec 29, 2016

eXon commented Jan 7, 2017 • edited Loading

andywer commented Jan 7, 2017

eXon commented Jan 7, 2017

eXon commented Jan 8, 2017

andywer commented Jan 11, 2017 • edited Loading

andywer commented Jan 11, 2017 • edited Loading

eXon commented Jan 11, 2017

andywer commented Jan 11, 2017

andywer commented Jan 11, 2017

eXon commented Jan 11, 2017

andywer commented Jan 11, 2017

eXon commented Jan 13, 2017

eXon commented Jan 7, 2017 •

edited

Loading

andywer commented Jan 11, 2017 •

edited

Loading

andywer commented Jan 11, 2017 •

edited

Loading