JSG_STRUCT_TS_OVERRIDE_DYNAMIC by refactoring JSG_STRUCT

[AdityaAtulTewari]

Enable Dynamic TS overrides

Goal:

Enable Dynamic typescript overrides that are based on compat flags

JSG_STRUCT(.........);
JSG_STRUCT_TS_OVERRIDE_DYNAMIC(CompatibilityFlags::Reader flags) {
  if(flags.getCacheEnabled()) {
    JSG_TS_OVERRIDE(...);
   else {
    JSG_TS_OVERRIDE(...);
   }
}

Mechanism:

We do this by refactoring JSG_STRUCT to be more extensible in line with JSG_RESOURCE_TYPE.
We create an internal function registerMembersInternal that enables no repeat code and we use c++20 requires in order to select which implementation of registerMembers should be selected.

Steps:

• Refactor JSG_STRUCT with an internal function
• Add JSG_STRUCT_TS_OVERRIDE_DYNAMIC(..) macro
• Create another INTERNAL macro that conditionally includes that behavior
• Test

Test plan

• Successful use in Http Request Cache Part 1 #2434
• Add another test to jsg-test.c++

[jasnell]

Any chance of a test?

[jasnell]

I'm good with this approach but would like @kentonv to review and sign off.

[AdityaAtulTewari]

Any chance of a test?

I want to add a test but how would it work? It is hard to automate a test to check whether or not types exist. I'm happy to do something if you can suggest how to test the overrides.

[kentonv]

Is the motivation for this to have RequestInitializerDict::cache's type change based on the newly-created compat flag?

Is that actually important? It seems like we can just define the type according to the standard either way -- but without the compat flag, it'll throw (at runtime) if you actually set it.

[AdityaAtulTewari]

Is the motivation for this to have RequestInitializerDict::cache's type change based on the newly-created compat flag?

Yes in the generated typescript.

Is that actually important? It seems like we can just define the type according to the standard either way -- but without the compat flag, it'll throw (at runtime) if you actually set it.

My motivation for this is to create total equality to the initial state so without the flag the users sees no difference. However @jasnell may have other reasons for this.

[kentonv]

My motivation for this is to create total equality to the initial state so without the flag the users sees no difference. However @jasnell may have other reasons for this.

IMO it's not important to prevent any observable change to the typescript types. The important thing is that we don't break workers in production, but typescript changes don't have any effect on what's already in production.

[jasnell]

... it's not important to prevent any observable change to the typescript types...

I disagree with this a bit. We've had a number of issues pop up due to observable changes in the types and it is something that we at least should be careful of. I don't feel strongly about this specific change, I had mentioned only to make sure any possible issues were considered. I would defer to the frameworks team (@IgorMinar @dario-piotrowicz @jculvey @vicb ) to weigh in on what to do here.

[kentonv]

I maintain that it is not important to ensure that RequestInitializerDict.cache is absent from the type declarations when the compat flag is not enabled. Having this declaration appear in the types won't break anyone in itself, even if attempts to use it throw an exception. Sure, there's a remote chance it causes some minor confusion for someone writing new code, but probably that confusion exists already as people are probably looking at MDN docs, where this property has been defined and documented all along.

So I don't think we need to bend over backwards adding new features to JSG just to avoid this.

[penalosa]

I maintain that it is not important to ensure that RequestInitializerDict.cache is absent from the type declarations when the compat flag is not enabled. Having this declaration appear in the types won't break anyone in itself, even if attempts to use it throw an exception. Sure, there's a remote chance it causes some minor confusion for someone writing new code, but probably that confusion exists already as people are probably looking at MDN docs, where this property has been defined and documented all along.

I don't really agree with this. The main idea behind all of our TypeScript generation work has been to make sure that the TS types match runtime behaviour as closely as possible (and the recent wrangler types improvements have gone further in that direction). If we can, it would be a much nicer DX to have .cache cause a type error with the wrong compat date, rather than relying on a runtime error. However, I'm not sure I really understand why this needs JSG changes—we already do this type of thing. For instance, the global navigator is only added when the right compat flag is enabled:

if (flags.getGlobalNavigator()) {
  JSG_LAZY_INSTANCE_PROPERTY(navigator, getNavigator);
  JSG_NESTED_TYPE(Navigator);
}

[jasnell]

@penalosa

... If we can, it would be a much nicer DX to have .cache cause a type error with the wrong compat date, rather than relying on a runtime error. However, I'm not sure I really understand why this needs JSG changes—we already do this type of thing. For instance, the global navigator is only added when the right compat flag is enabled:

the difference here is that JSG_RESOURCE_TYPE already implements the ability to vary the type output based on compat flags and JSG_STRUCT does not. We also have no way of conditionally handling the cache property on a JSG_STRUCT based on the compat flag settings. So the options right now are: (a) always expose the new types for cache regardless of whether the compat flag is set or (b) land the changes in this PR to make it respect the compat flag.

[kentonv]

The latest iteration of the code looks fine. Still question the necessity but I won't block it.