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 #3286

Merged
merged 4 commits into from Jun 10, 2022
Merged

fix: improve parsing of --env flag #3286

merged 4 commits into from Jun 10, 2022

Conversation

snitin315
Copy link
Member

What kind of change does this PR introduce?

bugfix.

Did you add tests for your changes?

Yes

If relevant, did you update the documentation?

I will add a note on webpack.js.org

Summary

Fix #3284

Set the variable to undefined instead of setting it to true.

$ npx webpack --env TEST1="" --env TEST2=$NON_EXISTENT_ENVVAR --env TEST3=
{
  WEBPACK_BUNDLE: true,
  WEBPACK_BUILD: true,
  'TEST1': '',
  'TEST2': undefined,
  'TEST3': undefined
}
asset main.bundle.js 50 bytes [compared for emit] [minimized] (name: main)
./main.js 28 bytes [built] [code generated]
webpack 5.73.0 compiled successfully in 82 ms

Does this PR introduce a breaking change?

No

Other information
No

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.

Let's add test case --foo=undefined, in this case we should have undefined string value

@codecov
Copy link

codecov bot commented Jun 9, 2022

Codecov Report

Merging #3286 (c43fa6d) into master (1072e38) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3286   +/-   ##
=======================================
  Coverage   92.08%   92.09%           
=======================================
  Files          23       23           
  Lines        1731     1733    +2     
  Branches      519      519           
=======================================
+ Hits         1594     1596    +2     
  Misses        137      137           
Impacted Files Coverage Δ
packages/webpack-cli/src/webpack-cli.ts 93.96% <100.00%> (+0.01%) ⬆️

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 1072e38...c43fa6d. Read the comment docs.

@snitin315 snitin315 marked this pull request as ready for review June 9, 2022 15:19
@snitin315 snitin315 requested a review from a team as a code owner June 9, 2022 15:19
// Should generate the appropriate files
expect(existsSync(resolve(__dirname, "./dist/undefined-foo.js"))).toBeTruthy();
});
}
Copy link
Member

Choose a reason for hiding this comment

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

And lets add

webpack --env TEST=""
webpack --env TEST=\\\"\\\"

To avoid future regressions

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 ✅

rishabh3112
rishabh3112 previously approved these changes Jun 10, 2022
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.

Looks good, left a suggestion.

Co-authored-by: Rishabh Chawla <rishabh31121999@gmail.com>
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

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, we need a patch release after merge

@snitin315 snitin315 merged commit 402c0fe into master Jun 10, 2022
@snitin315 snitin315 deleted the fix/env-parsing branch June 10, 2022 12:29
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.

Variables passed to --env with an empty value are incorrectly parsed
6 participants