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: resolve dev server hot options correctly #2022

Merged
merged 11 commits into from
Nov 4, 2020
Merged

Conversation

anshumanv
Copy link
Member

What kind of change does this PR introduce?
fix

Did you add tests for your changes?
Yes

If relevant, did you update the documentation?
Need update

Summary

  • Fix how hot flag is resolved in serve package

Does this PR introduce a breaking change?
No

Other information
Fixes #2004

@anshumanv anshumanv requested a review from a team as a code owner November 2, 2020 11:42
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.

Can we add simple test for webpack serve --hot and webpack serve --hot=only to avoid problems in future

Copy link
Member

@rishabh3112 rishabh3112 left a comment

Choose a reason for hiding this comment

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

Left a suggestion. Rest looks good to me 😃

packages/serve/src/createConfig.ts Show resolved Hide resolved
@anshumanv
Copy link
Member Author

anshumanv commented Nov 3, 2020

Can we add simple test for webpack serve --hot and webpack serve --hot=only to avoid problems in future

We already have tests for hot and no-hot flags, also updated the tests for createConfig which was causing this problem, adding for hot-only

Copy link
Member

@snitin315 snitin315 left a comment

Choose a reason for hiding this comment

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

Need a rebase

@alexander-akait
Copy link
Member

/cc @anshumanv can you rebase, I think we should do release today

@webpack-bot
Copy link

@anshumanv Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@snitin315 Please review the new changes.

@anshumanv
Copy link
Member Author

rebase done

alexander-akait
alexander-akait previously approved these changes Nov 4, 2020
@codecov
Copy link

codecov bot commented Nov 4, 2020

Codecov Report

Merging #2022 into master will increase coverage by 0.06%.
The diff coverage is 64.70%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2022      +/-   ##
==========================================
+ Coverage   68.99%   69.05%   +0.06%     
==========================================
  Files          85       85              
  Lines        2422     2443      +21     
  Branches      489      492       +3     
==========================================
+ Hits         1671     1687      +16     
- Misses        751      756       +5     
Impacted Files Coverage Δ
packages/serve/src/parseArgs.ts 90.47% <0.00%> (-4.53%) ⬇️
packages/serve/src/startDevServer.ts 93.33% <ø> (+2.02%) ⬆️
packages/serve/src/index.ts 84.61% <66.66%> (-15.39%) ⬇️
packages/serve/src/createConfig.ts 64.70% <77.77%> (+9.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 ec6d8b3...e4e4b86. 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.

/cc @webpack/cli-team

Will be great to add more tests, but let's focus on coverage in other PRs

@anshumanv anshumanv merged commit 7c5a2ba into webpack:master Nov 4, 2020
@anshumanv anshumanv deleted the 2004 branch November 4, 2020 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[serve] --hot-only is broken
5 participants