-
-
Notifications
You must be signed in to change notification settings - Fork 602
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
feat: built in env files support #3759
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should implement this as a plugin, so developers can reuse it, we need to impement this as a package
, but I am fine with starting it as a plugin inside webpack-cli package, the main idea get https://github.com/mrsteele/dotenv-webpack/blob/master/src/index.js and rework it:
- allow to set custom logic for loading dotenv files (by default we will use the same as you written)
- allow to set multiple
prefix
, because you can have ESM and common js files in projects, we need to allow setimport.meta.env
andprocess.env
- no
silent
, webpack has built-in logger, so let's use it - implement cache to avoid extra reading
and maybe other tweeak, yeah, the issue is not small (and not so big), so we can do it step by step
Creating a new folder under packages. |
Let's take |
@alexander-akait if we do want to distribute it as a standalone plugin in the future, isnt it better to add it to the webpack codebase or put it under webpack-contrib org? creating it as a plugin here and then moving it to a different repo to distribute it additional unnecessary work |
@burhanuday That's right, we can do it this way, but I'd rather keep it here for better integration with CLI (i.e. allows to have special flags for this) and fast fixes and fast releases, not sure it really make sense to use it as standalone plugin (except complex cases) |
Okay. Creating new package here then |
The main goal is a seamless integraiton, moving it and rename is not long task, implementation is the most long task |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3759 +/- ##
==========================================
+ Coverage 91.64% 91.77% +0.13%
==========================================
Files 22 23 +1
Lines 1628 1776 +148
Branches 466 505 +39
==========================================
+ Hits 1492 1630 +138
- Misses 136 146 +10
... and 5 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
since |
// files at the end of the array have higher priority | ||
envFiles = this.defaultFileList, | ||
prefixes = ["process.env.", "import.meta.env."], | ||
envVarPrefix = "PUBLIC_", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's allow it to be string | string[]
, some env variables can be from other environments, for example SENTRY_ (error reporting) requre prefix SENTRY_
, and it can be used in client, so you can have ["PUBLIC_", "SENTRY_"]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I deeply look at dotenv-plugin
and what we missing:
allowEmptyValues
, we really need it, it allows to catch bad cases- let's add test case for
const { DB_HOST, DB_PASS } = process.env;
(we supports it in webpack, just to check it), same forimport.meta
And I still think safe
is good idea, but we can postpone it
Good job, thank you, I think we will merge/release it on this week
In this plugin we are defining the variables using string like While figuring this out, i was reading the webpack docs for define plugin and saw that this is recommended against doing in a note on this link - https://webpack.js.org/plugins/define-plugin/#usage Other than that, i have implemented |
@alexander-akait friendly ping |
@burhanuday yeah, sorry for delay, on my TODOs list on the top after #3739, |
web-infra-dev/rspack#2208 this is also the requested features from rspack users, glad to see webpack-cli is willing to support it |
@hardfist Yeah, it will be an official part of webpack ecosystem, I want to be align with next.js logic, it is flexible and solves almost all requests, hope we will finish it soon (this week) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! Could you fix the .gitignore glob and I'll do a manual review after?
@@ -64,5 +64,7 @@ test/**/**/**/bin/ | |||
test/**/**/binary/** | |||
test/**/dist | |||
test/**/**/dist | |||
test/**/**/dist1 | |||
test/**/**/dist2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test/**/**/dist*
@burhanuday I think also we need to check how the new node --env-file option could be used alongside this. Wil review your PR soon! :) |
Х̶о̶р̶о̶ш̶о̶ с̶ н̶е̶т̶е̶р̶п̶е̶н̶и̶е̶м̶ ж̶д̶у̶
ср, 22 нояб. 2023 г., 03:04 Even Stensberg ***@***.***>:
… @burhanuday <https://github.com/burhanuday> I think also we need to check
how the new node --env-file option could be used alongside this. Wil review
your PR soon! :)
—
Reply to this email directly, view it on GitHub
<#3759 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BBO6JJWAWFJIWT3AXTUV5QTYFUCOXAVCNFSM6AAAAAAXNRXH5KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMRRGYYDCNBYGY>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
@ Thanks for your update. I labeled the Pull Request so reviewers will review it again. @snitin315 Please review the new changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you rebase?
What kind of change does this PR introduce?
feature
Adds built in support to webpack to handle .env files
Did you add tests for your changes?
not yet
If relevant, did you update the documentation?
not yet
Summary
Closes #3747
Does this PR introduce a breaking change?
No
Other information