add liveReload option to enable/disable refreshing the page #1812

EslamHiko · 2019-04-22T20:18:19Z

For Bugs and Features; did you add new tests?

I'm not sure how i would test this feature except using the browser

Motivation / Use-Case

fixes #1744

Breaking Changes

No

Additional Info

live reloading is enabled by default

codecov · 2019-04-22T20:36:04Z

Codecov Report

Merging #1812 into master will decrease coverage by 0.07%.
The diff coverage is 80%.

@@            Coverage Diff             @@
##           master    #1812      +/-   ##
==========================================
- Coverage   88.34%   88.26%   -0.08%     
==========================================
  Files          14       14              
  Lines         815      818       +3     
  Branches      258      260       +2     
==========================================
+ Hits          720      722       +2     
- Misses         82       83       +1     
  Partials       13       13

Impacted Files	Coverage Δ
lib/Server.js	`86.36% <100%> (+0.02%)`	⬆️
lib/utils/createConfig.js	`92.59% <50%> (-0.81%)`	⬇️

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 2f56b18...22d5c98. Read the comment docs.

alexander-akait

Need tests, thanks for PR, looks good

EslamHiko · 2019-04-24T19:51:04Z

@evilebottnawi I couldn't find any way to test the actual code except using the browser and testing it manually, so I had to create a simple simulation it does exactly like what the actual code does. if you have any idea or examples to write a test to check if the page reloads or not it'll be very helpful to write a better test.

EslamHiko · 2019-04-24T19:56:52Z

@evilebottnawi AppVeyor has a connection problem Could not resolve host: github.com Command exited with code 128 i think rerunning the test will solve it.

hiroppy · 2019-05-12T14:51:59Z

@EladBezalel We dropped travis and appveyor. Could you rebase?

EslamHiko · 2019-05-12T16:58:49Z

@hiroppy I did a mistake in rebasing, can you please tell me the right way to rebase? and is everything ok or should I fix something?

hiroppy · 2019-05-12T17:02:51Z

The rebasing seems to be successful. However, CI fails.

FAIL test/options.test.js (39.888s)
  ● options › should match properties and errorMessage

    expect(received).toEqual(expected) // deep equality

    Expected: 57
    Received: 58

      14 |     const messages = Object.keys(options.errorMessage.properties);
      15 | 
    > 16 |     expect(properties.length).toEqual(messages.length);
         |                               ^
      17 | 
      18 |     const res = properties.every((name) => messages.includes(name));
      19 | 

      at Object.toEqual (test/options.test.js:16:31)

Please update test/options.test.js

hiroppy · 2019-05-12T17:06:36Z

lib/options.json

@@ -4,6 +4,15 @@
    "after": {
      "instanceof": "Function"
    },
+    "serveIndex": {


Please delete here(master has already had serverIndex)

https://github.com/webpack/webpack-dev-server/blob/master/lib/options.json#L282

hiroppy · 2019-05-12T17:06:57Z

lib/options.json

+    "liveReload": {
+      "type": "boolean"
+    },
+    "hot": {


Please delete here(master has already had hot)

https://github.com/webpack/webpack-dev-server/blob/master/lib/options.json#L113

alexander-akait

Good job, small fixes

alexander-akait · 2019-05-13T10:30:53Z

lib/Server.js

-    this.headers = this.options.headers;
-    this.progress = this.options.progress;
+    this.hot = options.hot || options.hotOnly;
+    this.liveReload = options.liveReload;


Don't use this.liveReload (this variables will be removed in next major release), please use this.options.liveReload in code

alexander-akait · 2019-05-13T10:31:04Z

lib/Server.js

@@ -790,6 +791,10 @@ class Server {
          this.sockWrite([connection], 'progress', this.progress);
        }

+        if (this.liveReload !== false) {


this.options.liveReload

alexander-akait · 2019-05-13T10:31:15Z

lib/Server.js

-      this.sockWrite(this.sockets, 'content-changed');
-    });
+    // disabling refreshing on changing the content
+    if (this.liveReload !== false) {


this.options.liveReload

alexander-akait · 2019-05-13T12:14:07Z

lib/Server.js

@@ -95,7 +95,7 @@ class Server {

    // TODO this.<property> is deprecated (remove them in next major release.) in favor this.options.<property>
    this.hot = options.hot || options.hotOnly;
-    this.liveReload = options.liveReload;
+    this.options.liveReload = options.liveReload;


No need this line after refactor, we already have this.options.liveReload

alexander-akait

One note

alexander-akait

Good, let's wait CI green

EslamHiko · 2019-05-13T13:36:26Z

@evilebottnawi coverage tests didn't pass so I did a small fix maybe using options instead of this.options caused the coverage tests to fail

EslamHiko · 2019-05-14T11:40:28Z

@evilebottnawi anything i can modify or fix to pass the tests ?

alexander-akait

Need add test for https://github.com/webpack/webpack-dev-server/blob/master/test/CreateConfig.test.js, becase use should support --no-live-reload using CLI (no need tests for CLI)

alexander-akait · 2019-05-14T12:24:21Z

lib/utils/createConfig.js

@@ -88,6 +88,10 @@ function createConfig(config, argv, { port }) {
    options.hotOnly = argv.hotOnly;
  }

+  if (!options.liveReload) {
+    options.liveReload = argv.liveReload;
+  }


webpack-dev-server/lib/utils/createConfig.js

Line 114 in 9a86d02

else if (argv.contentBase === false) {

we should test --no-live-reload because be default liveReload should be true

alexander-akait

One note

alexander-akait

Looks good, due we don't have CLI tests, can you manually testing?

EslamHiko · 2019-05-14T12:48:38Z

@evilebottnawi I tested it and it's working with me.

alexander-akait · 2019-05-14T12:50:40Z

Awesome thanks!

EslamHiko · 2019-05-14T13:00:43Z

@evilebottnawi what went wrong? I think I missed to add a test to test false case in createConfig.test

alexander-akait · 2019-05-14T13:02:52Z

@EslamHiko Awesome === Great/Good

alexander-akait · 2019-05-14T13:03:20Z

Need update snapshot to fix CI problem

EslamHiko · 2019-05-14T13:10:01Z

@evilebottnawi I updated my snapshot.

hiroppy · 2019-05-15T19:47:23Z

@EslamHiko I did the following things

rebase and squash your commits (because we are going to squash commits before merging to master, and we have commitlint on CI)
~~delete updating snapshots commit~~
- I forgot that the client code has changed in this PR, so I updated snapshots

thanks

hiroppy · 2019-05-15T20:12:15Z

yay, CI is green 🍏

EslamHiko · 2019-05-15T20:16:52Z

@hiroppy 🎉 🎉 i hope everything is fine now 😄

alexander-akait

/cc @hiroppy approve?

alexander-akait · 2019-05-16T13:28:49Z

/cc @EslamHiko need rebase again, sorry

alexander-akait · 2019-05-16T16:47:26Z

Close in favor #1889

EslamHiko requested review from alexander-akait and hiroppy as code owners April 22, 2019 20:18

alexander-akait requested changes Apr 23, 2019

View reviewed changes

EslamHiko force-pushed the add-live-reload-option branch from 4d7afe0 to 0285f1d Compare May 12, 2019 16:30

hiroppy requested changes May 12, 2019

View reviewed changes

alexander-akait requested changes May 13, 2019

View reviewed changes

alexander-akait reviewed May 13, 2019

View reviewed changes

alexander-akait requested changes May 13, 2019

View reviewed changes

alexander-akait previously approved these changes May 13, 2019

View reviewed changes

EslamHiko dismissed alexander-akait’s stale review via 7f47bb7 May 13, 2019 13:32

alexander-akait reviewed May 14, 2019

View reviewed changes

alexander-akait requested changes May 14, 2019

View reviewed changes

alexander-akait previously approved these changes May 14, 2019

View reviewed changes

alexander-akait closed this May 14, 2019

alexander-akait reopened this May 14, 2019

This comment has been minimized.

Sign in to view

hiroppy force-pushed the add-live-reload-option branch from 46b1f70 to cadee8b Compare May 15, 2019 19:43

alexander-akait previously approved these changes May 16, 2019

View reviewed changes

EslamHiko added 13 commits May 16, 2019 16:29

add liveReload option to enable/disable refreshing the page

03f1dae

add tests

3b7a72d

delete unnecessary comments

6e56bc9

fix conflicts

3dd9206

solve conflict-2

252b73f

add livReload to errorMessages & remove duplicate options

d3149c1

use this.options.liveReload instead of this.liveReload

e858aa5

add cli support

bebf0f4

test no-live-reload

bb8acfa

update snapshot

ac6da29

fix debugging error

569c914

fix some typos

d86b591

update snapshot

92cddd7

EslamHiko dismissed alexander-akait’s stale review via 92cddd7 May 16, 2019 14:34

EslamHiko force-pushed the add-live-reload-option branch from 5245bcc to 92cddd7 Compare May 16, 2019 14:34

EslamHiko added 3 commits May 16, 2019 16:48

fix: fix conflict

ff3d872

test(routes): use master's snapshots

17e1fc0

Merge branch 'master' into add-live-reload-option

22d5c98

alexander-akait added this to the 3.4.0 milestone May 16, 2019

EslamHiko mentioned this pull request May 16, 2019

feat(server): add liveReload option to enable/disable live reloading #1889

Merged

6 tasks

alexander-akait closed this May 16, 2019

add liveReload option to enable/disable refreshing the page #1812

add liveReload option to enable/disable refreshing the page #1812

Conversation

EslamHiko commented Apr 22, 2019 • edited by alexander-akait

For Bugs and Features; did you add new tests?

Motivation / Use-Case

Breaking Changes

Additional Info

codecov bot commented Apr 22, 2019 • edited

Codecov Report

alexander-akait left a comment

Choose a reason for hiding this comment

EslamHiko commented Apr 24, 2019

EslamHiko commented Apr 24, 2019

hiroppy commented May 12, 2019

EslamHiko commented May 12, 2019

hiroppy commented May 12, 2019

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

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

alexander-akait left a comment

Choose a reason for hiding this comment

EslamHiko commented May 13, 2019 • edited

EslamHiko commented May 14, 2019

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

alexander-akait left a comment

Choose a reason for hiding this comment

EslamHiko commented May 14, 2019

alexander-akait commented May 14, 2019

EslamHiko commented May 14, 2019 • edited

alexander-akait commented May 14, 2019

alexander-akait commented May 14, 2019

EslamHiko commented May 14, 2019 • edited

This comment has been minimized.

This comment has been minimized.

hiroppy commented May 15, 2019 • edited

hiroppy commented May 15, 2019

EslamHiko commented May 15, 2019 • edited

alexander-akait left a comment

Choose a reason for hiding this comment

alexander-akait commented May 16, 2019

alexander-akait commented May 16, 2019

EslamHiko commented Apr 22, 2019 •

edited by alexander-akait

codecov bot commented Apr 22, 2019 •

edited

EslamHiko commented May 13, 2019 •

edited

EslamHiko commented May 14, 2019 •

edited

EslamHiko commented May 14, 2019 •

edited

hiroppy commented May 15, 2019 •

edited

EslamHiko commented May 15, 2019 •

edited