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

[EslamHiko]

• This is a bugfix
• This is a feature
• This is a code refactor
• This is a test update
• This is a docs update
• This is a metadata update

For Bugs and Features; did you add new tests?

Yes

Motivation / Use-Case

fix : #1831
Alternative to : #2133

Breaking Changes

The socket server will be disabled when hot and liveReload are disabled.

Additional Info

I updated the values of liveReload: false tests since hot became enabled by default.
I added tests for ws and moved socket-injection.test.js to e2e folder since e2e testing is the only way to test ws as it became the default websocket.
Also I've updated console.log test in test/e2e/ClientOptions.test.js to check for all WebSocket options.

Not sure if logging tests in test/e2e/ClientOptions.test.js are enough so I don't know whether I should delete test/e2e/socket-injection.test.js or not.

[codecov]

Codecov Report

Merging #2601 into v4 will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##               v4    #2601   +/-   ##
=======================================
  Coverage   92.34%   92.35%           
=======================================
  Files          34       34           
  Lines        1320     1321    +1     
  Branches      372      373    +1     
=======================================
+ Hits         1219     1220    +1     
  Misses         96       96           
  Partials        5        5           

Impacted Files	Coverage Δ
client-src/default/index.js	92.22% <ø> (ø)
lib/Server.js	95.90% <100.00%> (+<0.01%)	⬆️

---

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 76bcd84...a66d157. Read the comment docs.

[alexander-akait]

I think it is unnecessary here to write log.error('[WDS] Hot Module Replacement & Live Reloading are disabled!');

/cc @Loonride @hiroppy

[hiroppy]

Agree

[alexander-akait]

Put this.options.liveReload in normalizeOptions with code:

options.liveReload = typeof options.liveReload !== "undefined" ? options.liveReload : true;

[alexander-akait]

Good job, couple changes

[EslamHiko]

/cc @hiroppy @Loonride

[hiroppy]

Agree

[hiroppy]

You can use async/await already.

[EslamHiko]

@hiroppy I tried many times to use async/await but every time I get errors or wrong results, my last attempt was :

cases.forEach(async ({ title, options }) => {
      title += ` (${
        Object.keys(mode).length ? mode.transportMode : 'default'
      })`;
      options = { ...mode, ...options };
      testOptions = Object.assign({}, baseOptions, options);
      await testServer.startAwaitingCompilation(config, testOptions); // and without await
      it(title, (done) => {

        const res = [];
          runBrowser().then(({ page, browser }) => {
            page.goto(`http://localhost:${port2}/main`);
            page.on('console', ({ _text }) => {
              res.push(_text);
            });

            // wait for load before closing the browser
            page.waitForNavigation({ waitUntil: 'load' }).then(() => {
              page.waitFor(beforeBrowserCloseDelay).then(() => {
                browser.close().then(() => {
                  // Order doesn't matter, maybe we should improve that in future
                  expect(res.sort()).toMatchSnapshot();
                  testServer.close(done)
                });
              });
            });
          });
      });
    });

for now It works fine, I need help with rewriting this with async/await.
@Loonride any thoughts for this ?

[knagaitsev]

I believe the issue is forEach won't work with async, as in it won't wait for the previous iteration to finish before doing the next one, at the very top where you do cases.forEach. Try doing a normal for loop and see if that fixes it

[EslamHiko]

@Loonride thanks for the hint, what fixed the issue was adding await before it & adding the server starter inside it, not sure if converting the other promises helped ex :

await it(title, async (done) => {
        await testServer.startAwaitingCompilation(config, testOptions);
        const res = [];
        const { page, browser } = await runBrowser();
        page.goto(`http://localhost:${port2}/main`);
        page.on('console', ({ _text }) => {
          res.push(_text);
        });
        // wait for load before closing the browser
        await page.waitForNavigation({ waitUntil: 'load' });
        await page.waitFor(beforeBrowserCloseDelay);
        await browser.close();
        // Order doesn't matter, maybe we should improve that in future
        await expect(res.sort()).toMatchSnapshot();
        await testServer.close(done);
      });

It's strange that using for for the inner loop didn't work & skipped many tests.

[knagaitsev]

Looks like test/client/index.test.js snapshot needs updating

[EslamHiko]

/cc @evilebottnawi @hiroppy

[alexander-akait]

@EslamHiko Please fix conflicts, sorry for the situation 😄

[EslamHiko]

/cc @evilebottnawi

[alexander-akait]

/cc @hiroppy