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

temporal doesn't work in deno.json's unstable field #22123

Closed
RuiNtD opened this issue Jan 26, 2024 · 11 comments · Fixed by #22134
Closed

temporal doesn't work in deno.json's unstable field #22123

RuiNtD opened this issue Jan 26, 2024 · 11 comments · Fixed by #22134

Comments

@RuiNtD
Copy link

RuiNtD commented Jan 26, 2024

Version: Deno 1.40.1

Despite having temporal set in the unstable field in my deno.json, Temporal is undefined and requires me to provide --unstable-temporal.

{
  "unstable": ["temporal"]
}
error: Uncaught (in promise) ReferenceError: Temporal is not defined
  return Temporal.Duration.from(ttl).total({
  ^
@bartlomieju
Copy link
Member

Thanks for reporting. You hit a jackpot of complexity here! I didn't test this scenario explicitly and it turns out it's really hard to fix. That said, I'll think of a solution to fix it in v1.40.2.

@RuiNtD
Copy link
Author

RuiNtD commented Jan 26, 2024

You might want to see about refactoring the code a bit so that config.json unstable flags and CLI unstable flags are always defined in the same place.

@bartlomieju
Copy link
Member

You might want to see about refactoring the code a bit so that config.json unstable flags and CLI unstable flags are always defined in the same place.

That would be ideal, but various unstable features need to be initialized at different stages of the program lifetime - eg. temporal needs to be initialized at the very beginning, when we tell V8 that we're gonna start up. Sadly at the moment figuring out which features are enabled in config file happens way later, than initialization of V8.

@irbull
Copy link
Contributor

irbull commented Jan 26, 2024

I had a bit of time this evening so I thought I'd take a look, and you're right this is pretty complex. The feature_checker does have the right flags set, whether you use --unstable-temporal or a deno.json file.

What I'm unclear about is why is this particular flag different from cron or any of the other APIs. It seems to be configured in a different way than the other unstable APIs: https://github.com/denoland/deno/blob/main/runtime/lib.rs#L92.

@bartlomieju
Copy link
Member

I had a bit of time this evening so I thought I'd take a look, and you're right this is pretty complex. The feature_checker does have the right flags set, whether you use --unstable-temporal or a deno.json file.

What I'm unclear about is why is this particular flag different from cron or any of the other APIs. It seems to be configured in a different way than the other unstable APIs: https://github.com/denoland/deno/blob/main/runtime/lib.rs#L92.

That's because we need to pass a proper CLI flag to V8 to make this API available. All the other unstable APIs are our own APIs that we can enable/disable when we bootstrap the runtime. For temporal, this needs to happen before we even create a first V8 instance.

@irbull
Copy link
Contributor

irbull commented Jan 26, 2024

That's because we need to pass a proper CLI flag to V8 to make this API available. All the other unstable APIs are our own APIs that we can enable/disable when we bootstrap the runtime. For temporal, this needs to happen before we even create a first V8 instance.

So this is more of a V8 Flag then? I guess none of the other V8 flags can be specified in a deno.json file, right?

If that's the case, is there a reason why we can't process the deno.json file early in the startup process? Is it a performance thing or does the processing of the file require V8 (it seems to be done in Rust, but there could be calls into V8 that I'm not seeing).

@bartlomieju
Copy link
Member

That's because we need to pass a proper CLI flag to V8 to make this API available. All the other unstable APIs are our own APIs that we can enable/disable when we bootstrap the runtime. For temporal, this needs to happen before we even create a first V8 instance.

So this is more of a V8 Flag then? I guess none of the other V8 flags can be specified in a deno.json file, right?

--unstable-temporal is a Deno flag. To activate Temporal API we need to pass --harmony-temporal flag to V8 on startup. That's correct, other V8 flags can be specified only using --v8-flags=... CLI flag.

If that's the case, is there a reason why we can't process the deno.json file early in the startup process? Is it a performance thing or does the processing of the file require V8 (it seems to be done in Rust, but there could be calls into V8 that I'm not seeing).

We can do that, but it requires some bigger refactor that I'm working on right now.

@RuiNtD
Copy link
Author

RuiNtD commented Jan 26, 2024

We can do that, but it requires some bigger refactor that I'm working on right now.

It also requires processing JSONC in Rust, btw.

@bartlomieju
Copy link
Member

We can do that, but it requires some bigger refactor that I'm working on right now.

It also requires processing JSONC in Rust, btw.

I'm not sure what you mean. Configuration files already support JSONC.

@RuiNtD
Copy link
Author

RuiNtD commented Jan 26, 2024

We can do that, but it requires some bigger refactor that I'm working on right now.

It also requires processing JSONC in Rust, btw.

I'm not sure what you mean. Configuration files already support JSONC.

Sorry, I just assumed JSONC was handled in JavaScript and not in Rust.

@irbull
Copy link
Contributor

irbull commented Jan 26, 2024

Sorry, I just assumed JSONC was handled in JavaScript and not in Rust.

There is also a Rust implementation here: https://github.com/denoland/deno_config. There are a lot of goodies around Deno 😄 .

bartlomieju added a commit that referenced this issue Mar 8, 2024
Actual fix happened in #22782, but
this commit adds additional tests and cleans up V8 flags passed on init.

Closes #22123
Closes #22560
Closes #22557
littledivy pushed a commit that referenced this issue Mar 8, 2024
Actual fix happened in #22782, but
this commit adds additional tests and cleans up V8 flags passed on init.

Closes #22123
Closes #22560
Closes #22557
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 a pull request may close this issue.

3 participants