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: add support for the new server option #3940

Merged
merged 17 commits into from
Oct 27, 2021
Merged

feat: add support for the new server option #3940

merged 17 commits into from
Oct 27, 2021

Conversation

snitin315
Copy link
Member

@snitin315 snitin315 commented Oct 12, 2021 •
edited
Loading

  • 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?

WIP on tests.

Motivation / Use-Case

Add support for the server option. More priority than https option.

server: "http" | "https" | "spdy" | { type: "http" | "https" | "spdy", options }
webpack --server-type http
webpack --server-type https --server-options-key ./path/to/key
webpack --server-type http2 --server-options-key ./path/to/key

Breaking Changes

None

Additional Info

#3912 (comment)

alexander-akait
alexander-akait reviewed Oct 12, 2021
View reviewed changes
alexander-akait
alexander-akait reviewed Oct 15, 2021
View reviewed changes
test/cli/__snapshots__/basic.test.js.snap.webpack5 Outdated
@@ -119,7 +119,6 @@ Options:
--open-target-reset Clear all items provided in 'open.target' configuration. Opens specified page in browser.
--open-app-name-reset Clear all items provided in 'open.app.name' configuration. Open specified browser.
--port <value> Allows to specify a port to use.
--server <value> Allows to set server and options (by default 'http').
Copy link
Member

Choose a reason for hiding this comment

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

hm, why we remove it? We have --web-socket-server ws, so I think --server http2 should work too, same for --server https with auto generation for key/cert

Copy link
Member Author

Choose a reason for hiding this comment

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

Otherwise, we have two similar flags --server-type <value> and --server <value>.

Copy link
Member Author

Choose a reason for hiding this comment

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

for webSocketServer we have only --web-socket-server and not --web-socket-server-type as the object options aren't cli firendly.

Copy link
Member

Choose a reason for hiding this comment

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

hm, maybe you are right, we should check how webpack is doing the same for similar options (I think we already have the same cases in webpack), can you check?

Copy link
Member Author

Choose a reason for hiding this comment

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

I will check.

@codecov
Copy link

codecov bot commented Oct 16, 2021 •
edited
Loading

Codecov Report

Merging #3940 (294e85f) into master (4afc727) will increase coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3940      +/-   ##
==========================================
+ Coverage   92.58%   92.65%   +0.07%     
==========================================
  Files          14       14              
  Lines        1362     1376      +14     
  Branches      474      486      +12     
==========================================
+ Hits         1261     1275      +14     
  Misses         93       93              
  Partials        8        8
Impacted Files Coverage Δ
bin/cli-flags.js 100.00% <ø> (ø)
lib/Server.js 93.94% <100.00%> (+0.08%) ⬆️

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 4afc727...294e85f. Read the comment docs.

@alexander-akait
Copy link
Member

@snitin315 ready for review?

@snitin315
Copy link
Member Author

Yes

@snitin315 snitin315 marked this pull request as ready for review October 20, 2021 00:15
@alexander-akait
Copy link
Member

alexander-akait commented Oct 20, 2021 •
edited
Loading

@snitin315 Did you look how webpack handle enum and object for CLI options?

@snitin315
Copy link
Member Author

I looked at webpack schema, but I was unable to find a similar case.

@alexander-akait alexander-akait mentioned this pull request Oct 24, 2021
Merged
6 tasks
@alexander-akait
Copy link
Member

I will review tomorrow

@alexander-akait alexander-akait merged commit a70a7ef into master Oct 27, 2021
@alexander-akait alexander-akait deleted the feat/server-option branch October 27, 2021 16:58
@snitin315 snitin315 mentioned this pull request Oct 30, 2021
Merged
@weaverryan weaverryan mentioned this pull request May 31, 2022
Merged
weaverryan added a commit to symfony/webpack-encore that referenced this pull request Jul 6, 2022
@weaverryan
feature #1125 Changing to support the "server" options object for web…
275fb6f 
…pack-dev-server (weaverryan)

This PR was squashed before being merged into the main branch.

Discussion
----------

Changing to support the "server" options object for webpack-dev-server

Follow up to #1118 and solves #1119.

The new `server` object has existed since `webpack-dev-server` 4.4 webpack/webpack-dev-server#3940, and we already require 4.8.

Support for the old `https` kept for now (with deprecations) to help people migrate.

Commits
-------

e0fb0ad Changing to support the "server" options object for webpack-dev-server
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 left review comments

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

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

@knagaitsev knagaitsev Awaiting requested review from knagaitsev

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

Successfully merging this pull request may close these issues.
2 participants
@snitin315 @alexander-akait