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: improve parsing of --env flag #2643

Merged
merged 4 commits into from
Apr 19, 2021
Merged

fix: improve parsing of --env flag #2643

merged 4 commits into from
Apr 19, 2021

Conversation

snitin315
Copy link
Member

@snitin315 snitin315 commented Apr 18, 2021

What kind of change does this PR introduce?
bugfix

Did you add tests for your changes?
Yes

If relevant, did you update the documentation?
will add an example in webpack.js.org after merge.

Summary

Fixes #2642

Does this PR introduce a breaking change?
Maybe

Other information
No

@codecov
Copy link

codecov bot commented Apr 18, 2021

Codecov Report

Merging #2643 (b7447c9) into master (d2ab57d) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2643      +/-   ##
==========================================
+ Coverage   95.19%   95.21%   +0.02%     
==========================================
  Files          30       30              
  Lines        1498     1505       +7     
  Branches      429      430       +1     
==========================================
+ Hits         1426     1433       +7     
  Misses         72       72              
Impacted Files Coverage Δ
packages/webpack-cli/lib/webpack-cli.js 96.17% <100.00%> (+0.03%) ⬆️

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 d2ab57d...b7447c9. Read the comment docs.

@snitin315 snitin315 marked this pull request as ready for review April 18, 2021 14:18
rishabh3112
rishabh3112 previously approved these changes Apr 18, 2021
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.

lgtm

@rishabh3112
Copy link
Member

There is logical error, for any value containing = this will match, better to look for last char in the string and if it's = then set value as ""

Yup, good catch. One more thing we can do is to introduce some special character for empty string like we do in parser grammars. ` could be tried for this.

@@ -373,6 +373,11 @@ class WebpackCLI {
{
name: 'env',
type: (value, previous = {}) => {
// for https://github.com/webpack/webpack-cli/issues/2642
if (value.endsWith('=')) {
value.concat('""');
Copy link
Member

Choose a reason for hiding this comment

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

Something weird with coverage here

@webpack-bot
Copy link

@alexander-akait Thanks for your update.

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

@anshumanv Please review the new changes.

@alexander-akait
Copy link
Member

Added more tests + fix edge cases with --verison= (empty string)

@alexander-akait
Copy link
Member

After CI will be green I want to merge it, because we need finish #2632 and when finish testing on webpack-dev-server@4, after this we need do release (and webpack-dev-server release too)

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.

--env foo="" is true instead of being "".
5 participants