-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
chore: add DENO_FUTURE env var #22318
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.
Please add an integration test
@@ -987,6 +987,10 @@ impl CliOptions { | |||
} | |||
} | |||
|
|||
pub fn enable_future_features(&self) -> bool { | |||
std::env::var("DENO_FUTURE").is_ok() |
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.
maybe we should ensure that the parsed value results in boolean true?
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.
we only want to check for presence of the env. eg: this should work DENO_FUTURE=1
prior art:
Line 55 in 1ad754b
if std::env::var("DEBUG").is_ok() { |
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.
DENO_FUTURE=false
will also enable future features even though user explicitly disabled it.
Looks like there is inconsistency in the codebase right now:
Lines 54 to 57 in 83d72e5
// This env var might be set by notebook if std::env::var("DEBUG").is_ok() { logger::init(Some(log::Level::Debug)); } deno/cli/lsp/language_server.rs
Lines 1148 to 1152 in 1ad754b
let is_byonm = std::env::var("DENO_UNSTABLE_BYONM").as_deref() == Ok("1") || maybe_config_file .as_ref() .map(|c| c.has_unstable("byonm")) .unwrap_or(false);
Edit: i think it's better to go with style 2 here.
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.
Additionally reading of env vars should be done only once in Lazy
at the top level. With this setup we will read env var on each invocation of this function.
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.
LGTM apart from the nit
Closes #22315 ``` ~> DENO_FUTURE=1 target/debug/deno > globalThis.window undefined ```
Closes #22315