deno icon indicating copy to clipboard operation
deno copied to clipboard

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

Open RuiNtD opened this issue 2 years ago • 11 comments

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({
  ^

RuiNtD avatar Jan 26 '24 00:01 RuiNtD

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.

bartlomieju avatar Jan 26 '24 00:01 bartlomieju

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.

RuiNtD avatar Jan 26 '24 00:01 RuiNtD

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.

bartlomieju avatar Jan 26 '24 01:01 bartlomieju

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.

irbull avatar Jan 26 '24 04:01 irbull

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.

bartlomieju avatar Jan 26 '24 13:01 bartlomieju

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).

irbull avatar Jan 26 '24 15:01 irbull

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.

bartlomieju avatar Jan 26 '24 16:01 bartlomieju

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.

RuiNtD avatar Jan 26 '24 17:01 RuiNtD

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.

bartlomieju avatar Jan 26 '24 18:01 bartlomieju

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.

RuiNtD avatar Jan 26 '24 18:01 RuiNtD

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 😄 .

irbull avatar Jan 26 '24 18:01 irbull