fix(env): set mode after config loaded

[evenstensberg]

What kind of change does this PR introduce?
Fixes #3992

Did you add tests for your changes?
No
If relevant, did you update the documentation?
N/A
Summary
Make sure mode is set based on node_env
Does this PR introduce a breaking change?
No
Other information
N/A

[codecov]

Codecov Report

Merging #4009 (d453561) into master (d40cae4) will increase coverage by 0.02%.
Report is 24 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4009      +/-   ##
==========================================
+ Coverage   90.87%   90.90%   +0.02%     
==========================================
  Files          22       22              
  Lines        1688     1693       +5     
  Branches      486      490       +4     
==========================================
+ Hits         1534     1539       +5     
  Misses        154      154              

Files	Coverage Δ
packages/webpack-cli/src/webpack-cli.ts	92.64% <100.00%> (+0.03%)	⬆️

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d40cae4...d453561. Read the comment docs.

[snitin315]

Can we add a corresponding test case?

[Ivan198407]

Д̶а̶ к̶о̶н̶е̶ч̶н̶о̶, п̶о̶ч̶е̶м̶у̶ н̶е̶т̶... ср, 22 нояб. 2023 г., 13:53 Nitin Kumar ***@***.***>:

…

***@***.**** commented on this pull request. Can we add a corresponding test case? — Reply to this email directly, view it on GitHub <#4009 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BBO6JJT6ELJBSZFK6X22KO3YFWOPHAVCNFSM6AAAAAA7O4FPIOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONBTGY4DCMZSGY> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[alexander-akait]

It is a bad idea, node-env and mode are different things and we already support another solution

[aoberoi]

@alexander-akait can you clarify what the other solution is? maybe we just need to update the docs to reflect it.

[aoberoi]

After reading the code, it seems like the mode is set automatically after the developer's exported config function is run and returns.

Issues like the one @mgnsm has reported (#3992) are occurring because the documentation isn't describing exactly how NODE_ENV, the arguments to the configuration function, and mode are coordinated.

Based on the current docs it seems reasonable to expect the automatic application of NODE_ENV to mode to show up somewhere in the config function. I think its usually the case that developers who implement a config function want to use the mode value to implement other logic as well (enabling / disabling plugins, modifying other config values, etc). The fact that this automatic application happens after the config function runs seems -- less than helpful.

In order to implement logic based on NODE_ENV/mode in the config function, the developer is forced to essentially duplicate the logic that's already in the CLI solution. Which ironically renders the CLI solution moot because the developer could just assign that to mode in the returned config themselves. So who does the CLI solution serve? It seems like its really for developers who are not using a config function.

Beyond updating the docs to make the intended behavior and use cases clear, I don't have a suggestion on updating the implementation. I think removing the current solution would be a breaking change, so I'm not in favor of that. I think adding this change would be duplicative, so I'm not in favor of that. I think we just need better guidance.

[alexander-akait]

Works as expected due #3992 (comment), yes, we need to update our docs here, I will help

[alexander-akait]

@aoberoi Yeah, you are right, argv is just an object of passed arguments through CLI, I don't know why developers expected to see the mode there

No need to duplicate logic because you can use argv.nodeEnv or process.env.NODE_ENV, the last solution is more common and using a lot of developers

Anyway feel free to feedback