fix(hot): enable hot option as default #2546

hiroppy · 2020-04-28T08:58:13Z

For Bugs and Features; did you add new tests?

Updated.

Motivation / Use-Case

The hot option is as default from now on.

Breaking Changes

yes

Additional Info

codecov · 2020-04-28T09:03:52Z

Codecov Report

Merging #2546 into feature/upgrade-deps will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@                  Coverage Diff                  @@
##           feature/upgrade-deps    #2546   +/-   ##
=====================================================
  Coverage                 93.40%   93.40%           
=====================================================
  Files                        34       34           
  Lines                      1334     1335    +1     
  Branches                    380      381    +1     
=====================================================
+ Hits                       1246     1247    +1     
  Misses                       85       85           
  Partials                      3        3

Impacted Files	Coverage Δ
lib/utils/createConfig.js	`96.39% <ø> (-0.07%)`	⬇️
lib/Server.js	`96.39% <100.00%> (+0.02%)`	⬆️
lib/utils/addEntries.js	`100.00% <100.00%> (ø)`
lib/utils/updateCompiler.js	`100.00% <100.00%> (ø)`

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 023c194...1ce649c. Read the comment docs.

alexander-akait · 2020-04-28T11:12:40Z

lib/Server.js

+
+    if (this.hot) {
+      this.options.hot = true;
+    }


Why do not use this.options.hot only (avoid using this.hot)?

addEntries checks this.options.hot so we need to add true when this.hot is true. So you want to delete hotOnly(or this.hot) or hotOnly should be added true as well?

Can we rewrite code on using this.options.hot only. What do you think about - this.options.hot: boolean | "only"?

ok, understood. so we need to rewrite this line(

webpack-dev-server/lib/Server.js

Line 734 in 89ffb86

if (this.hot) {

). I'll update this.

Ideally - avoid using this.[propertyFromOptions] in favor this.options.[property] in all code, but we can do it in the separate PR

Yes, I'll work on this on Next branch.

Please remove this.hot in this PR in favor this.options.hot

alexander-akait · 2020-04-28T14:06:18Z

lib/Server.js

@@ -86,7 +97,6 @@ class Server {
    this.contentBaseWatchers = [];

    // TODO this.<property> is deprecated (remove them in next major release.) in favor this.options.<property>


Remove TODO

Why? this task hasn't been completed.

Oh, yes, let's keep it

hiroppy · 2020-04-28T14:15:15Z

I've deleted hotOnly option but CI will fail because e2e/client.js is broking now. I need to investigate it.

alexander-akait

Good job! One note, but we can keep it as is to avoid misleading

alexander-akait · 2020-04-28T14:17:55Z

client-src/default/index.js

@@ -15,7 +15,7 @@ const status = {
  currentHash: '',
 };
 const options = {
-  hot: false,
+  hot: true,


I think we should not change it, because it breaks hot: false logic, we run .hot() when hot enabled, but if we use hot: false we doesn't run .hot() and default value now is true, so it can break logic for hot: false

Good catch, I fixed it. The CI's error was because of this.

alexander-akait · 2020-04-28T15:31:34Z

test/e2e/TransportMode.test.js

+                      const swap = res[0];
+                      res[0] = res[1];
+                      res[1] = swap;
+                    }


Why we need it?

It was deleted. just for my debugging to resolve the CI issue. The reason is #2546 (comment).

hiroppy · 2020-04-28T15:45:28Z

TransportMode.test.js is really flaky. The order of snapshot often is not fine between v5 and v4.

Understood this thing. This error has already existed before. In this time, we had changed the default value from false to true so we were able to notice it.

hiroppy · 2020-04-28T16:36:37Z

test/e2e/TransportMode.test.js

@@ -14,18 +14,21 @@ describe('transportMode client', () => {
    {
      title: 'sockjs',
      options: {
+        hot: false,


This test has been set the value as false from before.(currently, we set the default value as true so we need to add this option to here)

hiroppy · 2020-04-28T16:52:30Z

@evilebottnawi CI is green, please review again.

alexander-akait

Good job, thanks!

alexander-akait · 2020-04-28T17:11:12Z

Oh, we merge it into feature/upgrade-deps 😞

* chore(deps): upgrade deps * style: run prettier * test: update * ci: remove Node@8 * test(cli): add windows support * chore(deps): downgrade puppeteer * chore(deps): downgrade some deps * fix(hot): enable hot option as default (#2546) BREAKING CHANGE: the `hot` option is `true` by default, the `hotOnly` option was removed in favor `{ hot: 'only' }` * fix: remove lazy and filename options (#2544) BREAKING CHANGE: `lazy` and `filename` options was removed

hiroppy requested review from alexander-akait and knagaitsev as code owners April 28, 2020 08:58

hiroppy force-pushed the feature/enable-hot-option branch from 1da4986 to 8860742 Compare April 28, 2020 09:40

fix(hot): enable hot option as a default

6f89bb6

hiroppy force-pushed the feature/enable-hot-option branch from 8860742 to 6f89bb6 Compare April 28, 2020 09:52

alexander-akait reviewed Apr 28, 2020

View reviewed changes

fix: delete hotOnly option

fac6b79

alexander-akait approved these changes Apr 28, 2020

View reviewed changes

alexander-akait reviewed Apr 28, 2020

View reviewed changes

test: fix e2e/client

0c74447

hiroppy changed the title ~~fix(hot): enable hot option as a default~~ fix(hot): enable hot option as default Apr 28, 2020

hiroppy force-pushed the feature/enable-hot-option branch 2 times, most recently from d313992 to 5bf0372 Compare April 28, 2020 15:18

fix(client): set hot:false as a default

d627b88

hiroppy force-pushed the feature/enable-hot-option branch from 5bf0372 to d627b88 Compare April 28, 2020 15:23

alexander-akait reviewed Apr 28, 2020

View reviewed changes

hiroppy force-pushed the feature/enable-hot-option branch 2 times, most recently from 754d081 to ba99d38 Compare April 28, 2020 16:29

test: update

1ce649c

hiroppy force-pushed the feature/enable-hot-option branch from ba99d38 to 1ce649c Compare April 28, 2020 16:32

hiroppy commented Apr 28, 2020

View reviewed changes

alexander-akait approved these changes Apr 28, 2020

View reviewed changes

alexander-akait merged commit 5e183c0 into feature/upgrade-deps Apr 28, 2020

alexander-akait deleted the feature/enable-hot-option branch April 28, 2020 17:10

hiroppy mentioned this pull request May 3, 2020

fix(dev-server): remove hotOnly webpack/webpack.js.org#3727

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(hot): enable hot option as default #2546

fix(hot): enable hot option as default #2546

hiroppy commented Apr 28, 2020 •

edited

codecov bot commented Apr 28, 2020 •

edited

alexander-akait Apr 28, 2020

hiroppy Apr 28, 2020 •

edited

alexander-akait Apr 28, 2020

hiroppy Apr 28, 2020 •

edited

alexander-akait Apr 28, 2020

hiroppy Apr 28, 2020

alexander-akait Apr 28, 2020 •

edited

alexander-akait Apr 28, 2020

hiroppy Apr 28, 2020

alexander-akait Apr 28, 2020

hiroppy commented Apr 28, 2020

alexander-akait left a comment

alexander-akait Apr 28, 2020 •

edited

hiroppy Apr 28, 2020

alexander-akait Apr 28, 2020

hiroppy Apr 28, 2020 •

edited

hiroppy commented Apr 28, 2020 •

edited

hiroppy Apr 28, 2020 •

edited

hiroppy commented Apr 28, 2020

alexander-akait left a comment

alexander-akait commented Apr 28, 2020

		@@ -86,7 +97,6 @@ class Server {
		this.contentBaseWatchers = [];

		// TODO this.<property> is deprecated (remove them in next major release.) in favor this.options.<property>

fix(hot): enable hot option as default #2546

fix(hot): enable hot option as default #2546

Conversation

hiroppy commented Apr 28, 2020 • edited

For Bugs and Features; did you add new tests?

Motivation / Use-Case

Breaking Changes

Additional Info

codecov bot commented Apr 28, 2020 • edited

Codecov Report

Choose a reason for hiding this comment

hiroppy Apr 28, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiroppy Apr 28, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-akait Apr 28, 2020 • 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

hiroppy commented Apr 28, 2020

alexander-akait left a comment

Choose a reason for hiding this comment

alexander-akait Apr 28, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiroppy Apr 28, 2020 • edited

Choose a reason for hiding this comment

hiroppy commented Apr 28, 2020 • edited

hiroppy Apr 28, 2020 • edited

Choose a reason for hiding this comment

hiroppy commented Apr 28, 2020

alexander-akait left a comment

Choose a reason for hiding this comment

alexander-akait commented Apr 28, 2020

hiroppy commented Apr 28, 2020 •

edited

codecov bot commented Apr 28, 2020 •

edited

hiroppy Apr 28, 2020 •

edited

hiroppy Apr 28, 2020 •

edited

alexander-akait Apr 28, 2020 •

edited

alexander-akait Apr 28, 2020 •

edited

hiroppy Apr 28, 2020 •

edited

hiroppy commented Apr 28, 2020 •

edited

hiroppy Apr 28, 2020 •

edited