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.

Sign up for GitHub

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

Jump to bottom

feat: allow to configure more client options via resource URL #4274

Merged
merged 20 commits into from
Jul 25, 2022
Merged

feat: allow to configure more client options via resource URL #4274

merged 20 commits into from
Jul 25, 2022

Conversation

snitin315
Copy link
Member

  • 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 February 13, 2022 03:43
@alexander-akait
Copy link
Member

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 March 3, 2022 00:47
@snitin315
Copy link
Member Author

Something wrong with tests here, looking into it.

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

codecov bot commented Apr 17, 2022 •
edited
Loading

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.

alexander-akait
alexander-akait reviewed Apr 17, 2022
View reviewed changes
test/e2e/__snapshots__/allowed-hosts.test.js.snap.webpack4 Outdated
@@ -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

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

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

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

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

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

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 13:03
snitin315
snitin315 commented May 16, 2022
View reviewed changes
test/client/index.test.js
test("should run onSocketMessage.hot", () => {
onSocketMessage.hot();

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

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.

alexander-akait
alexander-akait reviewed May 16, 2022
View reviewed changes
client-src/index.js Outdated
@@ -46,16 +46,40 @@ const options = {
};
const parsedResourceQuery = parseURL(__resourceQuery);

const enabledFeatures = [];
Copy link
Member

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

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 July 24, 2022 02:17
alexander-akait
alexander-akait approved these changes Jul 25, 2022
View reviewed changes
Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

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

@snitin315
Copy link
Member Author

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

@snitin315 snitin315 merged commit 216e3cb into master Jul 25, 2022
@snitin315 snitin315 deleted the feat/allow-more-client-opts branch July 25, 2022 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexander-akait alexander-akait alexander-akait approved these changes

@hiroppy hiroppy Awaiting requested review from hiroppy hiroppy is a code owner

@knagaitsev knagaitsev Awaiting requested review from knagaitsev

@anshumanv anshumanv Awaiting requested review from anshumanv anshumanv is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Client overlay/progress options not configurable via resource URL
2 participants
@snitin315 @alexander-akait