-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(server): add liveReload option to enable/disable live reloading #1889
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1889 +/- ##
==========================================
- Coverage 91.27% 88.03% -3.25%
==========================================
Files 14 14
Lines 814 819 +5
Branches 257 260 +3
==========================================
- Hits 743 721 -22
- Misses 67 84 +17
- Partials 4 14 +10
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One note, my mistake, and we can merge
test/ContentBase.test.js
Outdated
fs.writeFileSync(nestedFile, 'Heyo', 'utf8'); | ||
}, 1000); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move this test to own test file LiveReload.test.js
lib/Server.js
Outdated
@@ -95,6 +95,7 @@ class Server { | |||
this.hot = this.options.hot || this.options.hotOnly; | |||
this.headers = this.options.headers; | |||
this.progress = this.options.progress; | |||
this.liveReload = this.options.liveReload; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't add here new options, you can see
webpack-dev-server/lib/Server.js
Line 94 in 0523cfd
// TODO this.<property> is deprecated (remove them in next major release.) in favor this.options.<property> |
this.options.liveReload
everywhere
lib/Server.js
Outdated
@@ -719,6 +720,10 @@ class Server { | |||
this.sockWrite([connection], 'hot'); | |||
} | |||
|
|||
if (this.liveReload !== false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As written above please use this.options.liveReload
lib/Server.js
Outdated
}); | ||
|
||
// disabling refreshing on changing the content | ||
if (this.liveReload !== false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As written above please use this.options.liveReload
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found new problem, we already fix it in other PR, looks you lose code
@evilebottnawi is this problem has something to do with the snapshots? |
@EslamHiko should not |
7a4a5de
to
121a79c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, i think we need refactor tests, but let's do it in next PR
/cc @hiroppy feel free to merge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, I wait for CI.
Thanks |
Thanks so much for doing this! I just tested out the new version and everything works as I expect. |
For Bugs and Features; did you add new tests?
yes
Motivation / Use-Case
fix this pr : #1812
Breaking Changes
no
Additional Info
I hope everything works fine this time.