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

Provide mechanism for autopopulating node.js process.env #3311

Merged
merged 1 commit into from
Feb 25, 2025

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Jan 9, 2025

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)

@jasnell jasnell requested review from a team as code owners January 9, 2025 17:01
@jasnell jasnell force-pushed the jasnell/workerd-autopopulate-process-env branch 3 times, most recently from 122a5b8 to d08499e Compare January 9, 2025 21:36
@jasnell jasnell force-pushed the jasnell/workerd-autopopulate-process-env branch 2 times, most recently from 00c74b2 to f6f3f64 Compare January 10, 2025 15:12
@jasnell jasnell requested a review from anonrig January 10, 2025 15:15
@jasnell jasnell force-pushed the jasnell/workerd-autopopulate-process-env branch from f6f3f64 to 7f531ca Compare January 10, 2025 15:15
@jasnell jasnell force-pushed the jasnell/workerd-autopopulate-process-env branch from 7f531ca to 5356dc7 Compare January 10, 2025 15:54
@jasnell jasnell force-pushed the jasnell/workerd-autopopulate-process-env branch from e6519ab to 09a9009 Compare January 10, 2025 16:28
vicb
vicb previously requested changes Jan 10, 2025
Copy link
Contributor

@vicb vicb left a 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

@jasnell
Copy link
Member Author

jasnell commented Jan 10, 2025

I oppose this change of it exposed secrets to the world via process.ENV

To be clear... the change puts any TEXT binding on process.env. We have no way of knowing if a a TEXT binding contains something that is considered a secret or not. If tooling insists on adding secrets to the worker configuration using a TEXT binding then this would mean we just cannot populate process.env automatically at all right now because we have zero ability to differentiate if the value of the TEXT binding is secret or not.

@jasnell jasnell enabled auto-merge (squash) January 14, 2025 16:04
@jasnell jasnell disabled auto-merge January 14, 2025 18:21
@jasnell jasnell marked this pull request as draft January 14, 2025 18:21
@jasnell
Copy link
Member Author

jasnell commented Jan 14, 2025

Moved back to draft due to continued backend discussion on how the implementation intersects with non-nodejs-compat related aspects of the runtime.

Signed-off-by: James M Snell <jsnell@cloudflare.com>
@jasnell jasnell force-pushed the jasnell/workerd-autopopulate-process-env branch from 590cb18 to a36ee3c Compare February 25, 2025 20:32
@jasnell jasnell marked this pull request as ready for review February 25, 2025 20:32
@jasnell jasnell dismissed vicb’s stale review February 25, 2025 20:32

Objection is noted but decision has been made to move forward.

@jasnell
Copy link
Member Author

jasnell commented Feb 25, 2025

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.

Copy link
Collaborator

@irvinebroque irvinebroque 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 find a way to ship

@jasnell jasnell dismissed kentonv’s stale review February 25, 2025 20:45

Current approach meets the immediate need. Hypothetical additional cases can be looked at separately later.

@vicb
Copy link
Contributor

vicb commented Feb 25, 2025

Could a doc PR be prepared for this change?

@jasnell
Copy link
Member Author

jasnell commented Feb 25, 2025

There already is a doc PR cloudflare/cloudflare-docs#19187

@jasnell jasnell merged commit ebfedb9 into main Feb 25, 2025
17 checks passed
@jasnell jasnell deleted the jasnell/workerd-autopopulate-process-env branch February 25, 2025 22:17
@vicb
Copy link
Contributor

vicb commented Feb 26, 2025

What are the plans regarding:

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.

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?

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.

7 participants