Skip to content

Conversation

bartlomieju
Copy link
Member

Follow up to #29586 that
adds a DENO_NODE_CONDITIONS env var that allows to
specify conditional exports when the user can't control
CLI flags passed to the command (eg. on Deploy).

Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

See comment. It won’t work with an envfile

@@ -4574,6 +4574,7 @@ fn lock_args() -> [Arg; 3] {
fn node_conditions_arg() -> Arg {
Arg::new("unstable-node-conditions")
.long("unstable-node-conditions")
.env("DENO_NODE_CONDITIONS")
Copy link
Member

Choose a reason for hiding this comment

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

We need to add a lint rule somehow to ban this. Can you pattern off the other code so that env vars only get resolved after loading the env file?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure - how will this work when we stabilize these and they are no longer unstable?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess the same as with UnstableConfig, just stored on the Flags struct

@bartlomieju bartlomieju requested a review from dsherret June 24, 2025 19:31
Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

LGTM

@bartlomieju bartlomieju enabled auto-merge (squash) June 24, 2025 19:55
@bartlomieju bartlomieju merged commit bff0950 into denoland:main Jun 25, 2025
33 of 35 checks passed
@bartlomieju bartlomieju deleted the node_conditions_env_var branch June 25, 2025 11:34
@Lukortech
Copy link

Great to see those changes mentioned in the latest video update. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants