Skip to content
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: allow to configure more client options via resource URL #4274

Merged
merged 20 commits into from Jul 25, 2022

Conversation

snitin315
Copy link
Member

@snitin315 snitin315 commented Feb 11, 2022

  • 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 #4157
Fix #4156
allow to configure progress and overlay options via resource URL.

Breaking Changes

None

Additional Info

NO

@snitin315 snitin315 force-pushed the feat/allow-more-client-opts branch from e2c6cd6 to 9dc2767 Compare Feb 13, 2022
@alexander-akait
Copy link
Member

alexander-akait commented Feb 13, 2022

Let's fix test and I think we can merge it

@snitin315 snitin315 force-pushed the feat/allow-more-client-opts branch from 9dc2767 to da958ac Compare Mar 3, 2022
@snitin315
Copy link
Member Author

snitin315 commented Mar 6, 2022

Something wrong with tests here, looking into it.

@snitin315 snitin315 force-pushed the feat/allow-more-client-opts branch from cd72a5c to 37db989 Compare Apr 17, 2022
@snitin315 snitin315 marked this pull request as ready for review Apr 17, 2022
@codecov
Copy link

codecov bot commented Apr 17, 2022

Codecov Report

Merging #4274 (f6ea02a) into master (cffffba) will increase coverage by 0.01%.
The diff coverage is 60.60%.

@@            Coverage Diff             @@
##           master    #4274      +/-   ##
==========================================
+ Coverage   92.18%   92.20%   +0.01%     
==========================================
  Files          16       16              
  Lines        1600     1629      +29     
  Branches      604      615      +11     
==========================================
+ Hits         1475     1502      +27     
- Misses        114      116       +2     
  Partials       11       11              
Impacted Files Coverage Δ
client-src/utils/log.js 46.66% <11.11%> (-53.34%) ⬇️
client-src/index.js 81.74% <64.28%> (+5.02%) ⬆️
lib/Server.js 93.70% <100.00%> (+0.05%) ⬆️
lib/servers/WebsocketServer.js 94.87% <0.00%> (+5.12%) ⬆️

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 cffffba...f6ea02a. Read the comment docs.

@@ -38,6 +38,7 @@ exports[`allowed hosts check host headers should always allow value of the \`hos

exports[`allowed hosts should connect web socket client using "[::1] host to web socket server with the "auto" value ("sockjs"): console messages 1`] = `
Array [
"[webpack-dev-server] Overlay is enabled for both errors and warnings.",
Copy link
Member

@alexander-akait alexander-akait Apr 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is small spammy

Copy link
Member Author

@snitin315 snitin315 Apr 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, we can remove this log.

Copy link
Member Author

@snitin315 snitin315 Apr 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since overlay is enabled by default this log will be printed in the console by default. And what about the progress is enabled log? Should we remove that too?

Copy link
Member

@alexander-akait alexander-akait Apr 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, to be honestly, it looks like very spammy currently, maybe we can join all messages in one? So developer will look one message about dev server started with HMR/overlay/etc

Copy link
Member Author

@snitin315 snitin315 Apr 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, [webpack-dev-server] started with HMR, live reloading, overlay, progress enabled.

Copy link
Member

@alexander-akait alexander-akait Apr 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do it 👍

@snitin315 snitin315 force-pushed the feat/allow-more-client-opts branch from f8812c6 to c97db65 Compare May 14, 2022
test("should run onSocketMessage.hot", () => {
onSocketMessage.hot();

expect(log.log.info.mock.calls[0][0]).toMatchSnapshot();
Copy link
Member Author

@snitin315 snitin315 May 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we removed these logs from onSocketMessage.hot() because otherwise, we would have duplicate logs spamming the console.

@@ -46,16 +46,40 @@ const options = {
};
const parsedResourceQuery = parseURL(__resourceQuery);

const enabledFeatures = [];
Copy link
Member

@alexander-akait alexander-akait May 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory we will have a memory problem here, because we will put more and more, I think will:

  1. create general log function
  2. create enabledFeatures object and set true/false
  3. pass enabledFeatures to general log function

And it will allow to us do better logging:

[webpack-dev-server] Server started: Hot Module Replacement enabled, Live Reloading enabled, Overlay disabled.

I.e. we will output enabled and disabled, so developers can see enabled/disabled features

Copy link
Member Author

@snitin315 snitin315 Jul 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexander-akait Done, sorry for the delay.

@snitin315 snitin315 force-pushed the feat/allow-more-client-opts branch from 6d42620 to 782d815 Compare Jul 24, 2022
Copy link
Member

@alexander-akait alexander-akait left a comment

Looks good, but there are a lot of uncoverred lines... Maybe you can add couple tests?

@snitin315
Copy link
Member Author

snitin315 commented Jul 25, 2022

Yeah, I'll add more tests and fix coverage separately.

@snitin315 snitin315 merged commit 216e3cb into master Jul 25, 2022
16 of 17 checks passed
@snitin315 snitin315 deleted the feat/allow-more-client-opts branch Jul 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Client overlay/progress options not configurable via resource URL
2 participants