Insert client implementation from within Dev Server API #1933

knagaitsev · 2019-05-28T23:27:02Z

For Bugs and Features; did you add new tests?

Not yet, they will be necessary though

Motivation / Use-Case

The goal here is to make it possible for the Dev Server API to pass a local client implementation over to the client dynamically. From this, we can make the easy step of:

clientMode: 'path/to/client-implementation.js'

Breaking Changes

The client should work the same after this change. I'll need to confirm that applying the ProvidePlugin does not cause problems for the compiler.

Additional Info

I'm not sure this is the best solution to the problem. If there is a better way to pass in a script which the client source will rely on, please recommend it
Possibly a bad choice to have the "global" variable __webpack_dev_server_client__, not sure there is a better way though. Optimally, we could restrict this variable only to the client bundle that the Dev Server inserts.
Should we let the user implement exponential connect retries themselves, or keep it the way I have made it?

Edit:

also: safe to use class on client side?

hiroppy · 2019-05-29T01:56:42Z

We should add tests for clients before refactoring. If we don't, we don't know if it's really breaking changes.

knagaitsev · 2019-05-29T02:58:11Z

@hiroppy I know, I can write some tests. I just wanted to put the solution out for review and discussion, in case someone has a better idea of how to pass along the client implementation from the API to the client src.

knagaitsev · 2019-05-29T05:47:07Z

Is the only way of really testing the client to do puppeteer or other e2e tests?

hiroppy · 2019-05-29T06:18:23Z

Currently, only Client.test.js.(so just e2e)😞

I've started to test about client/default/index.js as unit test using jest's jsdom since yesterday because I want to refactor this code and investigate bugs. We need to test unit and e2e.

And, I created a new directory as client.

alexander-akait · 2019-05-29T10:52:42Z

lib/utils/getClientPath.js

+  return require.resolve('./../clients/SockJSClient');
+}
+
+module.exports = getClientPath;


It should be static method for SockJSClient, i.e.
BaseClient should have static getClientPath(options) { throw new Error('Need implementation'); } and
SockJSClient should have static getClientPath(options) { return require.resolve('./../clients/SockJSClient'); }
For other clients too

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

lib/utils/updateCompiler.js

+      const providePlugin = new webpack.ProvidePlugin({
+        __webpack_dev_server_client__: getClientPath(),
+      });
+      providePlugin.apply(compiler);


As i said above SockJSClient.getClientPath(options)

alexander-akait · 2019-05-29T10:54:29Z

client-src/default/socket.js

    if (handlers[msg.type]) {
      handlers[msg.type](msg.data);
    }
-  };
+  });


Will be great standardize client API, but we can do this in future not in this PR

So you want to revert this and have the new client files, but not use them yet?

Keep this as is in this PR, we discussion about this in future

alexander-akait

Some notes

codecov · 2019-05-29T16:09:11Z

Codecov Report

Merging #1933 into master will decrease coverage by 0.35%.
The diff coverage is 90.47%.

@@            Coverage Diff             @@
##           master    #1933      +/-   ##
==========================================
- Coverage    91.8%   91.45%   -0.36%     
==========================================
  Files          23       25       +2     
  Lines         903      924      +21     
  Branches      283      283              
==========================================
+ Hits          829      845      +16     
- Misses         71       76       +5     
  Partials        3        3

Impacted Files	Coverage Δ
lib/clients/SockJSClient.js	`100% <100%> (ø)`
lib/utils/updateCompiler.js	`100% <100%> (ø)`	⬆️
lib/clients/BaseClient.js	`33.33% <33.33%> (ø)`
client-src/default/utils/reloadApp.js	`95.65% <0%> (-4.35%)`	⬇️
lib/Server.js	`91.86% <0%> (-0.44%)`	⬇️

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 44a8cde...6ee95e1. Read the comment docs.

alexander-akait · 2019-05-29T16:23:51Z

Let's update snapshots (we will rewrite this tests in future)

knagaitsev · 2019-05-29T16:28:18Z

Let's update snapshots (we will rewrite this tests in future)

Update snapshots in this PR?

alexander-akait · 2019-05-29T16:31:29Z

Yes

alexander-akait · 2019-05-29T16:31:43Z

yarn test:only -- -u

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

Looks good to me, but on the other hand, I am afraid of bugs by this change(of course in the future too).
So we should add tests of the client code as soon as possible.

knagaitsev · 2019-05-30T21:34:01Z

@hiroppy Agreed. Separate PR for new client tests? I will wait for addition of your jest jsdom tests and then work on adding more.

hiroppy · 2019-05-30T22:01:37Z

@Loonride I think that it is good to separate PRs but at the next refactoring, I hope that a PR will have tests:)
I'm sorry but I don't have enough time for coding because I am on vacation now(from 24th to 14th), so maybe it will take some time to submit a PR about client test. Although I can review at night:) I think that it is fine if you add tests in the following directory hierarchy. In addition, I'm writing only default, so we won't conflict. Thanks.

#1933 (comment)

As a first step, I think that it is good to do a unit test.

knagaitsev · 2019-05-31T01:09:51Z

@hiroppy Do you mean I should do unit tests for the new components I've added (SockJSClient, etc.)? Because if you are doing tests for default/index.js, that is pretty much the only other client file that needs to be tested, except like default/socket.js.

I'll also add more e2e tests in Client.test.js, as I think we should have tests that:

Confirm reloading works in inline mode
Confirm hot/hotOnly works
Intercept the sockjs communications (and WS in the future) to check the messages being sent
Test error handling in case client gets disconnected from server

Would you want such tests in Client.test.js, or in the new client directory?

hiroppy · 2019-05-31T03:23:38Z

Do you mean I should do unit tests for the new components I've added (SockJSClient, etc.)?

if needed

if you are doing tests for default/index.js, that is pretty much the only other client file that needs to be tested, except like default/socket.js

I was going to mock this line(socket(socketUrl, onSocketMsg)) because it can be divided completely. And this is an e2e test. (now, I'm testing as unit tests)
https://github.com/webpack/webpack-dev-server/blob/master/client-src/default/index.js#L272

Would you want such tests in Client.test.js, or in the new client directory?

I created a client directory, I think Client.test.js targets to check a URL path. (the file name might have to be changed) Also, I thought that I needed to do a lot of testing, so I wanted to split the file.

knagaitsev · 2019-05-31T03:29:43Z

I am currently writing a complex e2e test, I will just put it in Client.test.js and then consult you later where to move it

hiroppy · 2019-05-31T03:31:13Z

Yeah, that's fine 👍

hiroppy · 2019-05-31T03:35:46Z

And we should test using ProvidePlugin before merging.

also: safe to use class on client side?

We use babel, so it's ok.

alexander-akait · 2019-06-01T16:11:57Z

Can you rebase too?

alexander-akait · 2019-06-01T16:12:21Z

Something strange with CI, let's try to rebase as i said above

knagaitsev · 2019-06-01T16:25:50Z

Can you rebase too?

When you say rebase does it matter if I do merge or rebase? Just curious.

alexander-akait · 2019-06-01T16:50:01Z

git rebase master

jsf-clabot · 2019-06-01T16:56:39Z

All committers have signed the CLA.

hiroppy · 2019-06-01T16:59:29Z

Looks to fail to rebase.

$ git pull upstream master
$ git checkout inject-client
$ git rebase master inject-client

First, you should run git reflog and git reset --hard revision.

knagaitsev · 2019-06-01T17:35:59Z

I think I mistakenly reset to HEAD while in the middle of the rebase which just messed everything up.

alexander-akait · 2019-06-01T17:39:25Z

Yes, you can open new PR if you don't know how fix this problem

knagaitsev · 2019-06-01T17:40:49Z

Yes, you can open new PR if you don't know how fix this problem

I think it is good now, just need to update Routes snapshot again.

codecov · 2019-06-01T18:01:20Z

Codecov Report

Merging #1933 into master will decrease coverage by 0.03%.
The diff coverage is 90.47%.

@@            Coverage Diff             @@
##           master    #1933      +/-   ##
==========================================
- Coverage    91.8%   91.77%   -0.04%     
==========================================
  Files          23       25       +2     
  Lines         903      924      +21     
  Branches      283      283              
==========================================
+ Hits          829      848      +19     
- Misses         71       73       +2     
  Partials        3        3

Impacted Files	Coverage Δ
lib/clients/SockJSClient.js	`100% <100%> (ø)`
lib/utils/updateCompiler.js	`100% <100%> (ø)`	⬆️
lib/clients/BaseClient.js	`33.33% <33.33%> (ø)`

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 44a8cde...6ee95e1. Read the comment docs.

codecov · 2019-06-01T18:02:59Z

Codecov Report

Merging #1933 into master will decrease coverage by 0.04%.
The diff coverage is 90.47%.

@@            Coverage Diff             @@
##           master    #1933      +/-   ##
==========================================
- Coverage   92.42%   92.38%   -0.05%     
==========================================
  Files          25       27       +2     
  Lines         964      985      +21     
  Branches      307      307              
==========================================
+ Hits          891      910      +19     
- Misses         70       72       +2     
  Partials        3        3

Impacted Files	Coverage Δ
lib/clients/SockJSClient.js	`100% <100%> (ø)`
lib/utils/updateCompiler.js	`100% <100%> (ø)`	⬆️
lib/clients/BaseClient.js	`33.33% <33.33%> (ø)`

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 6973e4c...536a11d. Read the comment docs.

alexander-akait · 2019-06-03T15:52:43Z

/cc @Loonride need rebase
/cc @hiroppy need review

hiroppy

LGTM

knagaitsev requested review from alexander-akait and hiroppy as code owners May 28, 2019 23:27

knagaitsev changed the title ~~Insert client implementation as entry~~ Insert client implementation from Dev Server API May 28, 2019

knagaitsev changed the title ~~Insert client implementation from Dev Server API~~ Insert client implementation from within Dev Server API May 28, 2019

alexander-akait reviewed May 29, 2019

View reviewed changes

alexander-akait requested changes May 29, 2019

View reviewed changes

knagaitsev mentioned this pull request May 31, 2019

test(client): add e2e reload client tests #1940

Merged

6 tasks

knagaitsev closed this Jun 1, 2019

knagaitsev reopened this Jun 1, 2019

knagaitsev force-pushed the inject-client branch from 951302c to 05c1089 Compare June 1, 2019 17:33

knagaitsev closed this Jun 1, 2019

knagaitsev reopened this Jun 1, 2019

knagaitsev added 5 commits June 3, 2019 11:12

refactor(client): insert client implementation as entry

0e95cdd

refactor(client): removed entry of client, renamed helper

631e76b

refactor(client): made static method to get client path

376d470

test(client): added e2e ProvidePlugin tests

c80536c

test(client): added SockJSClient test

536a11d

knagaitsev force-pushed the inject-client branch from 6ee95e1 to 536a11d Compare June 3, 2019 16:47

hiroppy approved these changes Jun 3, 2019

View reviewed changes

alexander-akait approved these changes Jun 3, 2019

View reviewed changes

alexander-akait merged commit eecc1e4 into webpack:master Jun 3, 2019

alexander-akait mentioned this pull request Jun 6, 2019

[critical] webpack-dev-server broken in old browsers #1981

Closed

2 tasks

knagaitsev added gsoc Google Summer of Code scope: ws(s) labels Aug 13, 2019

Insert client implementation from within Dev Server API #1933

Insert client implementation from within Dev Server API #1933

Conversation

knagaitsev commented May 28, 2019 • edited Loading

For Bugs and Features; did you add new tests?

Motivation / Use-Case

Breaking Changes

Additional Info

hiroppy commented May 29, 2019

knagaitsev commented May 29, 2019

knagaitsev commented May 29, 2019

hiroppy commented May 29, 2019 • edited Loading

alexander-akait May 29, 2019

Choose a reason for hiding this comment

alexander-akait May 29, 2019

Choose a reason for hiding this comment

alexander-akait May 29, 2019

Choose a reason for hiding this comment

knagaitsev May 29, 2019

Choose a reason for hiding this comment

alexander-akait May 29, 2019

Choose a reason for hiding this comment

alexander-akait left a comment

Choose a reason for hiding this comment

codecov bot commented May 29, 2019 • edited Loading

Codecov Report

alexander-akait commented May 29, 2019

knagaitsev commented May 29, 2019

alexander-akait commented May 29, 2019

alexander-akait commented May 29, 2019

hiroppy commented May 30, 2019

knagaitsev commented May 30, 2019

hiroppy commented May 30, 2019 • edited Loading

knagaitsev commented May 31, 2019 • edited Loading

hiroppy commented May 31, 2019 • edited Loading

knagaitsev commented May 31, 2019

hiroppy commented May 31, 2019

hiroppy commented May 31, 2019 • edited Loading

alexander-akait commented Jun 1, 2019

alexander-akait commented Jun 1, 2019

knagaitsev commented Jun 1, 2019

alexander-akait commented Jun 1, 2019

jsf-clabot commented Jun 1, 2019 • edited Loading

hiroppy commented Jun 1, 2019 • edited Loading

knagaitsev commented Jun 1, 2019

alexander-akait commented Jun 1, 2019

knagaitsev commented Jun 1, 2019

codecov bot commented Jun 1, 2019 • edited Loading

Codecov Report

codecov bot commented Jun 1, 2019 • edited Loading

Codecov Report

alexander-akait commented Jun 3, 2019

hiroppy left a comment

Choose a reason for hiding this comment

knagaitsev commented May 28, 2019 •

edited

Loading

hiroppy commented May 29, 2019 •

edited

Loading

codecov bot commented May 29, 2019 •

edited

Loading

hiroppy commented May 30, 2019 •

edited

Loading

knagaitsev commented May 31, 2019 •

edited

Loading

hiroppy commented May 31, 2019 •

edited

Loading

hiroppy commented May 31, 2019 •

edited

Loading

jsf-clabot commented Jun 1, 2019 •

edited

Loading

hiroppy commented Jun 1, 2019 •

edited

Loading

codecov bot commented Jun 1, 2019 •

edited

Loading

codecov bot commented Jun 1, 2019 •

edited

Loading