fix: disable socket injection when hot & liveReload are disabled #2133

EslamHiko · 2019-07-18T11:34:52Z

For Bugs and Features; did you add new tests?

Yes

Motivation / Use-Case

Solves : #1831

Breaking Changes

No

Additional Info

codecov · 2019-07-18T11:39:01Z

Codecov Report

Merging #2133 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2133      +/-   ##
==========================================
+ Coverage   92.56%   92.58%   +0.01%     
==========================================
  Files          33       33              
  Lines        1265     1268       +3     
  Branches      361      363       +2     
==========================================
+ Hits         1171     1174       +3     
  Misses         87       87              
  Partials        7        7

Impacted Files	Coverage Δ
lib/Server.js	`97.35% <100%> (ø)`	⬆️
client-src/default/index.js	`92.39% <100%> (+0.16%)`	⬆️

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 5c5024c...324b006. Read the comment docs.

alexander-akait · 2019-07-18T12:04:47Z

test/e2e/__snapshots__/Iframe.test.js.snap

@@ -24,7 +24,8 @@ Array [

 exports[`Client iframe console.log liveReload disabled 1`] = `
 Array [
-  "Hey.",
+  "Failed to load resource: the server responded with a status of 404 (Not Found)",
+  "Failed to load resource: the server responded with a status of 404 (Not Found)",


~~Looks something wrong~~ expected

yes liveReload is disabled and hot is disabled too that's why the WebSocket is not working

I think it is possible that this could be a variable number of 404 error messages, since the client keeps retrying to connect. Maybe this should be handled some way for this particular test?

@Loonride any idea to handle these 404 errors?

@Loonride any idea to handle these 404 errors?

@EslamHiko Sorry I missed your reply, it seems tests generally pass as the timing is consistent to only output the error message twice, but just in case:

You could check that every element in that array is the same. Then make the snapshot only test the first element of the output array. This will allow for a variable number of the same error message.

alexander-akait

Good job! One note

alexander-akait

/cc @hiroppy @Loonride

knagaitsev · 2019-07-18T18:59:50Z

test/client/clients/WebsocketClient.test.js

@@ -60,3 +66,99 @@ describe('WebsocketClient', () => {
    });
  });
 });
+
+describe('websocket client injection', () => {


This should be moved to the test/server directory. Either in Server.test.js, or possibly a new test file of its own. @evilebottnawi thoughts on where it should go exactly?

@EslamHiko yep, let's move this test to test/server
@Loonride maybe test/server/hot-and-liveReload-integration, no idea how better naming this 😄

@Loonride @evilebottnawi okay, will move it to test/server/socket-injection what do you think about the name? 😅

test/server/socket-injection.test.js 👍

alexander-akait

One note and we can merge this 👍

alexander-akait · 2019-07-19T12:33:12Z

test/server/socket-injection.test.js

+          done();
+        });
+    });
+  });


Let's add test where hot: false, liveReload: true for explicit check

alexander-akait

/cc @hiroppy @Loonride

EslamHiko · 2019-07-23T21:11:00Z

test/e2e/__snapshots__/ClientOptions.test.js.snap

@@ -25,6 +25,9 @@ Array [
 exports[`Client console.log liveReload disabled 1`] = `
 Array [
  "Hey.",
+  "Failed to load resource: the server responded with a status of 404 (Not Found)",
+  "[WDS] Disconnected!",


should I add a check to disable "[WDS] Disconnected!" message when hot & liveReload are disabled ? @Loonride @evilebottnawi

I think yes, better output message like [WDS] hot and live reload was disables

alexander-akait · 2019-07-24T13:10:03Z

client-src/default/index.js

-    log.error('[WDS] Disconnected!');
+    if (options.hot || options.liveReload) log.error('[WDS] Disconnected!');
+    else
+      {log.error('[WDS] Hot Module Replacement & Live Reloading are disabled!');}


if (condition) { // Logic } else { // Logic }

codecov · 2019-07-24T17:36:10Z

Codecov Report

Merging #2133 into v4 will increase coverage by 0.16%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##               v4    #2133      +/-   ##
==========================================
+ Coverage   93.77%   93.93%   +0.16%     
==========================================
  Files          34       34              
  Lines        1333     1287      -46     
  Branches      381      368      -13     
==========================================
- Hits         1250     1209      -41     
+ Misses         81       77       -4     
+ Partials        2        1       -1

Impacted Files	Coverage Δ
client-src/default/index.js	`92.39% <100.00%> (+0.16%)`	⬆️
lib/Server.js	`97.32% <100.00%> (+0.53%)`	⬆️
lib/servers/WebsocketServer.js	`89.47% <0.00%> (-4.65%)`	⬇️
lib/utils/routes.js	`92.50% <0.00%> (-0.98%)`	⬇️
lib/utils/runOpen.js	`100.00% <0.00%> (ø)`
lib/utils/addEntries.js	`100.00% <0.00%> (ø)`
lib/utils/normalizeOptions.js	`100.00% <0.00%> (ø)`
client-src/default/utils/createSocketUrl.js	`100.00% <0.00%> (ø)`
client-src/clients/WebsocketClient.js	`57.89% <0.00%> (+0.39%)`	⬆️
... and 2 more

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 375ab23...0787620. Read the comment docs.

alexander-akait · 2019-09-16T14:02:40Z

/cc @hiroppy @Loonride

hiroppy · 2019-09-16T14:45:20Z

lib/Server.js

@@ -737,7 +737,9 @@ class Server {
    this.hostname = hostname;

    return this.listeningApp.listen(port, hostname, (err) => {
-      this.createSocketServer();
+      if (this.options.hot || this.options.liveReload !== false) {


- if (this.options.hot || this.options.liveReload !== false) { + if (this.options.hot || this.options.liveReload) {

@hiroppy sorry for my late reply but I think if I changed it, it won't create the socket server if options.liveReload was undefined the default value. I suggest making little refactor and write this.options.liveReload = this.options.liveReload !== false at the beginning of the constructor and replace all this.options.liveReload !== false with this.options.liveReload.

/cc @hiroppy

ok, we will fix this in the next version.

knagaitsev · 2019-09-25T17:13:22Z

@evilebottnawi Is it possible this should be on next? It is a good idea, but it is conceivable that users might still want the socket server running but do not have the right server settings. For example, someone could have the server running and are expecting to connect to the socket with a custom client not served by webpack-dev-server.

alexander-akait · 2019-09-25T18:38:15Z

@Loonride hm, what is use case for this action?

knagaitsev · 2019-10-02T02:21:54Z

@Loonride hm, what is use case for this action?

I can't think of a good use case. I was thinking hypothetically someone could connect to the socket server without using the served client, but there is likely no use for this.

Anyway, LGTM.

EslamHiko · 2019-11-08T12:08:57Z

/cc @hiroppy @evilebottnawi @Loonride CI is green 😄

alexander-akait · 2019-11-08T12:16:41Z

/cc @EslamHiko we merge this for next release because #2133 (comment), anyway thanks for PR, we will try to release next version on next/next-next week (we are working on webpack-dev-middleware)

* chore(deps): upgrade chokidar * chore(deps): switch to promise method without async for close * chore(ci): remove node v6 * chore(deps): fix issue of closing watchers before middleware * chore(deps): upgrade chokidar to v3.4.0

* 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 (webpack#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 (webpack#2544) BREAKING CHANGE: `lazy` and `filename` options was removed

…ck#2552)

BREAKING CHANGE: switch default transportMode to ws

…nd requestCert to https object (webpack#2564)

BREAKING CHANGE: the `client` now in `default` directory

BREAKING CHANGE: the `setup` was removed, `before` and `after` options were renamed to `onBeforeSetupMiddleware` and `onAfterSetupMiddleware`

alexander-akait · 2020-05-12T17:18:17Z

/cc @EslamHiko Can you send a PR for v4?

EslamHiko · 2020-05-13T06:20:13Z

@evilebottnawi I failed to rebase & created a fresh PR here: #2601

EslamHiko requested review from alexander-akait and hiroppy as code owners July 18, 2019 11:34

alexander-akait reviewed Jul 18, 2019

View reviewed changes

alexander-akait requested changes Jul 18, 2019

View reviewed changes

alexander-akait reviewed Jul 18, 2019

View reviewed changes

knagaitsev reviewed Jul 18, 2019

View reviewed changes

alexander-akait requested changes Jul 19, 2019

View reviewed changes

alexander-akait previously approved these changes Jul 19, 2019

View reviewed changes

wizardofhogwarts previously approved these changes Jul 21, 2019

View reviewed changes

EslamHiko dismissed stale reviews from wizardofhogwarts and alexander-akait via f8674e0 July 23, 2019 20:58

EslamHiko commented Jul 23, 2019

View reviewed changes

alexander-akait reviewed Jul 24, 2019

View reviewed changes

EslamHiko closed this Jul 24, 2019

EslamHiko reopened this Jul 24, 2019

hiroppy reviewed Sep 16, 2019

View reviewed changes

knagaitsev and others added 6 commits April 27, 2020 14:21

chore(deps): upgrade chokidar (webpack#2539)

d85c473

* chore(deps): upgrade chokidar * chore(deps): switch to promise method without async for close * chore(ci): remove node v6 * chore(deps): fix issue of closing watchers before middleware * chore(deps): upgrade chokidar to v3.4.0

fix(run-open): introduce open instead of opn (webpack#2532)

4e9b3db

test: support webapck5 (webpack#2553)

f5498c9

chore(deps): update package-lock.json (webpack#2551)

d29ddee

BREAKING CHANGE(server): get rid of conditions of Node version (webpa…

ecab24b

…ck#2552)

knagaitsev and others added 6 commits May 1, 2020 20:48

BREAKING CHANGE: remove inline and iframe/live mode (webpack#2550)

308ee21

feat: switch default transportMode to ws (webpack#2531)

83d4ec3

BREAKING CHANGE: switch default transportMode to ws

BREAKING CHANGE(https): migrate ca, cert, pfx, key, pfx-passphrase, a…

05d0a2b

…nd requestCert to https object (webpack#2564)

refactor: reorganize client directory (webpack#2562)

98b4a53

BREAKING CHANGE: the `client` now in `default` directory

fix(server): remove setup and rename before and after (webpack#2570)

97ccd22

BREAKING CHANGE: the `setup` was removed, `before` and `after` options were renamed to `onBeforeSetupMiddleware` and `onAfterSetupMiddleware`

chore(deps): update deps (webpack#2577)

768ced4

EslamHiko added 7 commits May 13, 2020 04:15

fix: disable socket injection when hot & liveReload are disabled

bee1748

test: move tests to test/server/socket-injection.test

819b31b

test: add more tests

f17cef5

test: update clientOptions snapshot

05d2cc2

test: change error message when hot and liveReload are disabled

edd5796

test: fix condition styling & a typo

3449175

test: add tests for ws & update results

0787620

EslamHiko changed the base branch from master to v4 May 13, 2020 05:20

EslamHiko force-pushed the disable-sock-injection branch from 80abcb2 to 0787620 Compare May 13, 2020 05:20

EslamHiko changed the base branch from v4 to master May 13, 2020 05:24

EslamHiko changed the base branch from master to v4 May 13, 2020 05:25

EslamHiko mentioned this pull request May 13, 2020

fix: disable socket injection when hot & liveReload are disabled (V4) #2601

Merged

6 tasks

EslamHiko closed this May 13, 2020

ylemkimon mentioned this pull request Nov 24, 2020

feat: remove CLI progress and move progress option into client #2857

Merged

6 tasks

Uh oh!

fix: disable socket injection when hot & liveReload are disabled #2133

fix: disable socket injection when hot & liveReload are disabled #2133

Uh oh!

Conversation

EslamHiko commented Jul 18, 2019

For Bugs and Features; did you add new tests?

Motivation / Use-Case

Breaking Changes

Additional Info

Uh oh!

codecov bot commented Jul 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

alexander-akait Jul 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexander-akait left a comment

Choose a reason for hiding this comment

Uh oh!

alexander-akait left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexander-akait left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexander-akait left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexander-akait Jul 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jul 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

alexander-akait commented Sep 16, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hiroppy Sep 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knagaitsev commented Sep 25, 2019

Uh oh!

alexander-akait commented Sep 25, 2019

Uh oh!

knagaitsev commented Oct 2, 2019

codecov bot commented Jul 18, 2019 •

edited

Loading

alexander-akait Jul 18, 2019 •

edited

Loading

alexander-akait Jul 24, 2019 •

edited

Loading

codecov bot commented Jul 24, 2019 •

edited

Loading

hiroppy Sep 24, 2019 •

edited

Loading

EslamHiko commented May 13, 2020 •

edited

Loading