-
Notifications
You must be signed in to change notification settings - Fork 343
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
Provide mechanism for autopopulating node.js process.env #3311
Conversation
122a5b8
to
d08499e
Compare
00c74b2
to
f6f3f64
Compare
f6f3f64
to
7f531ca
Compare
7f531ca
to
5356dc7
Compare
e6519ab
to
09a9009
Compare
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.
@irvinebroque can we discuss this?
I oppose this change of it exposed secrets to the world via process.env
To be clear... the change puts any |
Moved back to draft due to continued backend discussion on how the implementation intersects with non-nodejs-compat related aspects of the runtime. |
dc005bb
to
590cb18
Compare
Signed-off-by: James M Snell <jsnell@cloudflare.com>
590cb18
to
a36ee3c
Compare
Objection is noted but decision has been made to move forward.
Rebased the PR and expanded comments. Maintained the existing implementation approach. If it becomes necessary it ought to be able to further change the underlying implementation details without breaking changes. The hypothetical case around how to handle potential new binding types that evaluate to strings can be addressed separately later when they are no longer hypothetical. An internal PR is necessary. |
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 find a way to ship
Current approach meets the immediate need. Hypothetical additional cases can be looked at separately later.
Could a doc PR be prepared for this change? |
There already is a doc PR cloudflare/cloudflare-docs#19187 |
What are the plans regarding:
When this will be activated on EW? When can this be enabled? Does wrangler needs to be updated to support the flag/CLI options, is there a tracking issue? |
Autopopulates the process.env from bindings in local dev. A similar PR will be needed internally to enable it there as it won't be automatic.
See caveats: #3311 (comment)