feat/refactor(server): move socket handling into server.listen #2061

knagaitsev · 2019-06-24T21:40:26Z

For Bugs and Features; did you add new tests?

Yes

Motivation / Use-Case

This is the socket option handling portion of: #2028. The goal is to move socket handling out of the CLI and into the API listen method.

Breaking Changes

API users will now get different behavior when they put a socket path in place of port in the server.listen(port, hostname, fn) method.

Additional Info

This should be merged before I start work on the findPort portion of #2028

codecov · 2019-06-24T21:44:01Z

Codecov Report

Merging #2061 into next will increase coverage by 0.14%.
The diff coverage is 97.87%.

@@            Coverage Diff             @@
##             next    #2061      +/-   ##
==========================================
+ Coverage   96.05%   96.19%   +0.14%     
==========================================
  Files          34       35       +1     
  Lines        1191     1236      +45     
  Branches      346      349       +3     
==========================================
+ Hits         1144     1189      +45     
  Misses         46       46              
  Partials        1        1

Impacted Files	Coverage Δ
lib/Server.js	`97.21% <100%> (+0.09%)`	⬆️
lib/utils/startUnixSocket.js	`96.55% <96.55%> (ø)`
lib/utils/status.js	`95% <0%> (+5%)`	⬆️

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 cf6ea3f...8577191. Read the comment docs.

alexander-akait

Good job, just question

alexander-akait · 2019-06-25T09:15:34Z

lib/Server.js

+            userCallback(err);
+          });
+        }
+      });


Can we refactor this on utils? Maybe multiple utils

alexander-akait · 2019-06-25T18:21:18Z

lib/Server.js

+        socket,
+        setupAndListenCallback,
+        userCallback
+      );


Let rewrite this in style (pseudo code):

startUnixSocket( this.listeningApp, socket, (error) => { // Here logic for callback } );

Idea is not pass a lot of argument in util, just use closure for this

@evilebottnawi I was passing in 2 callbacks because I didn't want options.onListening to be called if an error occurred before the server starts listening. But maybe that is not important. If you think it is important, maybe we could have:

startUnixSocket( this.listeningApp, socket, (error, startedListening) => { if (startedListening) { this.options.onListening(this); } // other functions that always get called, regardless of if server started listening } );

hiroppy · 2019-06-25T23:00:08Z

This pr has breaking changes so I think we should change from master to next.

alexander-akait

hm, after investigation, looks like yes, it is breaking change, yes we should use next branch for this, anyway thanks for the PR

knagaitsev · 2019-06-27T16:26:02Z

Should I switch it over to next branch?

alexander-akait · 2019-06-27T16:36:53Z

Yes

hiroppy · 2019-06-27T18:10:02Z

@Loonride Could you rebase?

hiroppy · 2019-07-03T21:35:32Z

@Loonride Do you need commits of the latest master before merging? If you need, I'll rebase the next branch from master.

knagaitsev · 2019-07-03T21:42:40Z

@hiroppy For this PR the latest master commits should not affect it

hiroppy · 2019-07-03T21:49:24Z

After all, I'll rebase from the master to the next branch every time. master...next
I just want to ask if you want to check if these changes can run on the latest master commits branch before merging. However, if you don't need the latest master commits, it's ok.

hiroppy · 2019-08-06T22:48:39Z

lib/utils/startUnixSocket.js

+    });
+  };
+
+  fs.access(socket, fs.constants.F_OK, (e) => {


We don't need to use a callback. Just use promisify.

@hiroppy is this what you meant?:

https://github.com/webpack/webpack-dev-server/pull/2061/files#diff-2e3826007762dc4188ae8303ccd8b036R32-R38

Yes, thanks!

hiroppy · 2019-08-08T17:26:46Z

test/server/utils/startUnixSocket.test.js

+      setTimeout(() => {
+        expect(userCallback).toBeCalledTimes(1);
+        done();
+      }, 10000);


I think this value is big but do we need 10000?

I think 3000 should be enough.

hiroppy · 2019-08-10T18:36:54Z

Need to rebase.

alexander-akait · 2019-08-13T11:47:43Z

lib/Server.js

+      setupCallback();
+      userCallback(err);
+      onListeningCallback();
+    };


I think we should refactor this in future, looks very misleading

Really those 3 functions can all be in fullCallback, since they are only called by fullCallback. Originally I was doing some other things with these individual functions to avoid a few breaking changes and stay organized, but do you think just putting all their functionality in that one callback will be cleaner?

@Loonride i think we should allow userCallback to be async, also ehre good questions should userCallback will be called before or after onListeningCallback, potentially user callback should be called after all operation, it is allow do something with server (or not with server) when webpack-dev-server starts, for example for other web server, (for example if you use php project, you should use this callback to run php dev server)

i think we should allow userCallback to be async

Maybe we should do this in a separate PR?

potentially user callback should be called after all operation

So this is a good order the way I have above, because userCallback is after setupCallback, right?

Maybe we should do this in a separate PR?

Yes, just thoughts

So this is a good order the way I have above, because userCallback is after setupCallback, right?

oh, it is interesting, i think userCallback should be after all callbacks, because i want to get socket information after starts server

/cc @hiroppy what do you think about this too

I want to change onListening(err, server). In fact, this format is correct as Node.js theory.

So can we get this merged, and then I'll do 2 PRs:

async userCallback

onListening(err, server)

But @evilebottnawi can you clarify the async one? A user can already do this without problems:

server.listen(port, host, async (err) => { await something(); // ... });

And I think there are no things before userCallback that are asynchronous. Maybe you mean we should do:

setupCallback(); await userCallback(err); onListeningCallback();

But the only thing this would do is delay onListeningCallback until a potentially async userCallback is complete.

Is this what you meant?

But @evilebottnawi can you clarify the async one? A user can already do this without problems:

server.listen(port, host, async (err) => { await something(); // ... });

Yep, but it is require to create own util for running webpack-dev-server, in some cases it is unnecessary, because you want run one simple task when webpack-dev-server started

But the only thing this would do is delay onListeningCallback until a potentially async userCallback is complete.

Is this what you meant?

It is expected and can be used for some features

alexander-akait · 2019-08-22T10:50:02Z

Good job!

knagaitsev requested review from alexander-akait and hiroppy as code owners June 24, 2019 21:40

knagaitsev mentioned this pull request Jun 24, 2019

feat/refactor(server): move CLI server start into API listen method #2028

Closed

6 tasks

alexander-akait reviewed Jun 25, 2019

View reviewed changes

alexander-akait reviewed Jun 26, 2019

View reviewed changes

knagaitsev changed the base branch from master to next June 27, 2019 16:40

knagaitsev force-pushed the socket-option-check branch from 298317b to 580993b Compare June 28, 2019 14:47

knagaitsev mentioned this pull request Jul 29, 2019

[NEXT]: webpack-cli serve refactor draft webpack/webpack-cli#1011

Merged

knagaitsev added the pr: next label Jul 29, 2019

hiroppy force-pushed the next branch 2 times, most recently from b5c454d to 148dfd5 Compare August 2, 2019 15:54

knagaitsev force-pushed the socket-option-check branch from 580993b to 166ef18 Compare August 2, 2019 22:44

hiroppy force-pushed the next branch from 148dfd5 to d0893f1 Compare August 5, 2019 17:44

knagaitsev force-pushed the socket-option-check branch from 166ef18 to 7779cfb Compare August 6, 2019 20:46

hiroppy requested changes Aug 6, 2019

View reviewed changes

hiroppy reviewed Aug 8, 2019

View reviewed changes

knagaitsev added 4 commits August 12, 2019 17:52

refactor(server): move socket handling into server.listen

22dfb29

refactor(server): added startUnixSocket helper

4e52766

test(server): added unix socket helper tests

30c2d4f

test(server): fixed unix socket helper close method

bbdd01e

knagaitsev added 3 commits August 12, 2019 17:52

feat(server): made startUnixSocket take one callback

8da9a8c

refactor(server): use promisify to make unix socket async await

b352448

test(server): reduce time to wait for unix socket listening

8577191

knagaitsev force-pushed the socket-option-check branch from 0adc304 to 8577191 Compare August 12, 2019 22:54

hiroppy approved these changes Aug 13, 2019

View reviewed changes

alexander-akait approved these changes Aug 13, 2019

View reviewed changes

alexander-akait reviewed Aug 13, 2019

View reviewed changes

knagaitsev added gsoc Google Summer of Code scope: cli labels Aug 13, 2019

alexander-akait approved these changes Aug 22, 2019

View reviewed changes

alexander-akait merged commit 7ebb26a into webpack:next Aug 22, 2019

hiroppy pushed a commit that referenced this pull request Sep 10, 2019

refactor(server): move socket handling into server.listen (#2061)

64036a0

hiroppy pushed a commit that referenced this pull request Dec 12, 2019

refactor(server): move socket handling into server.listen (#2061)

7af06d9

hiroppy pushed a commit that referenced this pull request Dec 18, 2019

refactor(server): move socket handling into server.listen (#2061)

dbded36

hiroppy pushed a commit that referenced this pull request Dec 19, 2019

refactor(server): move socket handling into server.listen (#2061)

ea69c60

hiroppy pushed a commit that referenced this pull request Dec 23, 2019

refactor(server): move socket handling into server.listen (#2061)

f048b2e

hiroppy pushed a commit that referenced this pull request Dec 24, 2019

refactor(server): move socket handling into server.listen (#2061)

e52fa70

alexander-akait pushed a commit that referenced this pull request Jan 29, 2020

refactor(server): move socket handling into server.listen (#2061)

445f8d5

alexander-akait pushed a commit that referenced this pull request Jan 31, 2020

refactor(server): move socket handling into server.listen (#2061)

7509365

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat/refactor(server): move socket handling into server.listen #2061

feat/refactor(server): move socket handling into server.listen #2061

knagaitsev commented Jun 24, 2019 •

edited

Loading

codecov bot commented Jun 24, 2019 •

edited

Loading

alexander-akait left a comment

alexander-akait Jun 25, 2019

knagaitsev Jun 25, 2019

alexander-akait Jun 25, 2019

knagaitsev Jun 25, 2019

hiroppy commented Jun 25, 2019

alexander-akait left a comment •

edited

Loading

knagaitsev commented Jun 27, 2019

alexander-akait commented Jun 27, 2019

hiroppy commented Jun 27, 2019

hiroppy commented Jul 3, 2019 •

edited

Loading

knagaitsev commented Jul 3, 2019

hiroppy commented Jul 3, 2019 •

edited

Loading

hiroppy Aug 6, 2019

knagaitsev Aug 8, 2019

hiroppy Aug 8, 2019

hiroppy Aug 8, 2019

knagaitsev Aug 8, 2019

hiroppy commented Aug 10, 2019

alexander-akait Aug 13, 2019

knagaitsev Aug 15, 2019

alexander-akait Aug 15, 2019

knagaitsev Aug 16, 2019

alexander-akait Aug 16, 2019

alexander-akait Aug 16, 2019

hiroppy Aug 16, 2019

alexander-akait Aug 16, 2019

knagaitsev Aug 16, 2019 •

edited

Loading

alexander-akait Aug 20, 2019

alexander-akait commented Aug 22, 2019

feat/refactor(server): move socket handling into server.listen #2061

feat/refactor(server): move socket handling into server.listen #2061

Conversation

knagaitsev commented Jun 24, 2019 • edited Loading

For Bugs and Features; did you add new tests?

Motivation / Use-Case

Breaking Changes

Additional Info

codecov bot commented Jun 24, 2019 • edited Loading

Codecov Report

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

hiroppy commented Jun 25, 2019

alexander-akait left a comment • edited Loading

Choose a reason for hiding this comment

knagaitsev commented Jun 27, 2019

alexander-akait commented Jun 27, 2019

hiroppy commented Jun 27, 2019

hiroppy commented Jul 3, 2019 • edited Loading

knagaitsev commented Jul 3, 2019

hiroppy commented Jul 3, 2019 • edited Loading

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

hiroppy commented Aug 10, 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

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

knagaitsev Aug 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-akait commented Aug 22, 2019

knagaitsev commented Jun 24, 2019 •

edited

Loading

codecov bot commented Jun 24, 2019 •

edited

Loading

alexander-akait left a comment •

edited

Loading

hiroppy commented Jul 3, 2019 •

edited

Loading

hiroppy commented Jul 3, 2019 •

edited

Loading

knagaitsev Aug 16, 2019 •

edited

Loading