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

chore: add DENO_FUTURE env var #22318

Merged
merged 5 commits into from
Feb 15, 2024
Merged

Conversation

littledivy
Copy link
Member

Closes #22315

~> DENO_FUTURE=1 target/debug/deno

> globalThis.window
undefined

Copy link
Member

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

cli/worker.rs Outdated Show resolved Hide resolved
@littledivy littledivy enabled auto-merge (squash) February 15, 2024 04:39
@@ -987,6 +987,10 @@ impl CliOptions {
}
}

pub fn enable_future_features(&self) -> bool {
std::env::var("DENO_FUTURE").is_ok()
Copy link
Member

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?

Copy link
Member Author

@littledivy littledivy Feb 15, 2024

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:

if std::env::var("DEBUG").is_ok() {

Copy link
Member

@satyarohith satyarohith Feb 15, 2024

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:

  1. // This env var might be set by notebook
    if std::env::var("DEBUG").is_ok() {
    logger::init(Some(log::Level::Debug));
    }
  2. 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.

Copy link
Member

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.

Copy link
Member

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

@littledivy littledivy merged commit b72f0be into denoland:main Feb 15, 2024
17 checks passed
@littledivy littledivy deleted the deno_window_env branch February 15, 2024 04:50
littledivy added a commit that referenced this pull request Feb 15, 2024
Closes #22315

```
~> DENO_FUTURE=1 target/debug/deno

> globalThis.window
undefined
```
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.

Add DENO_FUTURE env var for testing breaking changes
3 participants