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

fix(cli): use webpack-cli@4 #2845

Merged
merged 5 commits into from
Nov 23, 2020
Merged

fix(cli): use webpack-cli@4 #2845

merged 5 commits into from
Nov 23, 2020

Conversation

ylemkimon
Copy link
Contributor

@ylemkimon ylemkimon commented Nov 19, 2020

  • 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

  • Added setupExitSignals option. Takes a boolean and if true (default on CLI), the server will close and exit the process on SIGINT and SIGTERM
  • server.listen() will find free port if this.options.port is not set and port argument is not passed
  • Print a warning if options.port and port passed as an argument is different
  • Added CLI calling webpack serve.

Breaking Changes

  • server.listen() now returns a Promise, which will resolve to server.listeningApp, after finding the free port.
  • webpack-dev-server CLI, utils/createConfig, and utils/processOptions are removed.

Additional Info

webpack-cli PR: webpack/webpack-cli#2126

Comment on lines +81 to +83
preprocess() {
process.argv.splice(2, 0, 'serve');
},
Copy link
Contributor Author

@ylemkimon ylemkimon Nov 19, 2020

Choose a reason for hiding this comment

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

This file is copied from webpack/bin/webpack.js and these lines were added.

@codecov
Copy link

codecov bot commented Nov 19, 2020

Codecov Report

Merging #2845 (3db0cea) into v4 (a16bbee) will decrease coverage by 0.38%.
The diff coverage is 81.81%.

Impacted file tree graph

@@            Coverage Diff             @@
##               v4    #2845      +/-   ##
==========================================
- Coverage   93.01%   92.62%   -0.39%     
==========================================
  Files          39       38       -1     
  Lines        1302     1247      -55     
  Branches      355      326      -29     
==========================================
- Hits         1211     1155      -56     
  Misses         87       87              
- Partials        4        5       +1     
Impacted Files Coverage Δ
lib/Server.js 95.47% <77.27%> (-0.85%) ⬇️
lib/utils/setupExitSignals.js 92.85% <90.90%> (-7.15%) ⬇️

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 a16bbee...3db0cea. Read the comment docs.

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.

We need improve this, but yes, it is very good job

lib/utils/setupExitSignals.js Show resolved Hide resolved
lib/utils/findPort.js Outdated Show resolved Hide resolved
bin/webpack-dev-server.js Outdated Show resolved Hide resolved
@alexander-akait
Copy link
Member

I need to go home (tired 😞 ), let's finish tomorrow, do you have time? Just ping me when it will be ready for review

@ylemkimon
Copy link
Contributor Author

@evilebottnawi No worries, whenever you want. This PR is ready for review.

@alexander-akait
Copy link
Member

Just note - let's add test on output using snapshot, we need to make sure your console output looks good, just start server via CLI do snapshot, change something and do snapshot again, I am afraid we can have some incompatibilities with webpack-cli

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@ylemkimon ylemkimon closed this Nov 23, 2020
@ylemkimon ylemkimon reopened this Nov 23, 2020
@ylemkimon ylemkimon closed this Nov 23, 2020
@ylemkimon ylemkimon reopened this Nov 23, 2020
@ylemkimon ylemkimon closed this Nov 23, 2020
@ylemkimon ylemkimon reopened this Nov 23, 2020
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.

Good job, thanks, I found a lot of strange deps like - find-cache-dir/util/url/many be other, can you look at code and maybe remove some of them, because I found them are unnecessary

@alexander-akait alexander-akait merged commit ccdce35 into webpack:v4 Nov 23, 2020
@ylemkimon
Copy link
Contributor Author

@evilebottnawi find-cache-dir is used to store temporary self-signed certificates:

// Use a self-signed certificate if no certificate was configured.
// Cycle certs every 24 hours
const certificateDir =
findCacheDir({ name: 'webpack-dev-server' }) || os.tmpdir();
const certificatePath = path.join(certificateDir, 'server.pem');
util and url are polyfills of Node modules for browsers.

@ylemkimon
Copy link
Contributor Author

Thank you for the review!

@alexander-akait
Copy link
Member

@ylemkimon Found other bug(s) with CLI, we have https://github.com/webpack/webpack-dev-server/blob/master/bin/cli-flags.js and https://github.com/webpack/webpack-dev-server/blob/master/bin/options.js, webpack serve use cli-flags.js, also we don't supports all types, like --open without options, we need to exports all possible types, like require('webpack').cli.getArguments do it, I think it is the last problem with CLI

@ylemkimon
Copy link
Contributor Author

ylemkimon commented Nov 24, 2020

v4's cli-flags seem to be set correctly, e.g., --open aceepts both Boolean and String:

{
name: 'open',
type: [String, Boolean],

@alexander-akait
Copy link
Member

alexander-akait commented Nov 24, 2020

Oh, yes , I see, anyway we have one extra file, let's remove it

@ylemkimon
Copy link
Contributor Author

Where was bin/options.js originally used?

@alexander-akait
Copy link
Member

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.

3 participants