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

Overrides for TypeScript definition generation from JSG RTTI #133

Merged
merged 3 commits into from
Nov 11, 2022

Conversation

mrbbot
Copy link
Contributor

@mrbbot mrbbot commented Oct 27, 2022

Hey! 👋 This is a follow-up PR to cloudflare/workers-types#113, enabling auto-generated TypeScript definitions to be customised with "overrides". These are based on how autodecl used to work, however overrides are now written alongside the original C++ definitions using macros. This should mean overrides don't get out-of-sync with the original definitions.

In particular, 6 new macros have been added:

  • JSG_(STRUCT_)TS_ROOT: start visiting types for inclusion from this type
  • JSG_(STRUCT_)TS_OVERRIDE: customise the generated definition for this type by merging or replacing it with a partial TypeScript definition
  • JSG_(STRUCT_)TS_DEFINE: insert additional handwritten TypeScript next to the generated definition for this type

See the documentation in src/workerd/jsg/jsg.h for more explanation on each.

Arguments to these macros are stored verbatim in the encoded RTTI Cap'n Proto file, and are parsed using the TypeScript Compiler API in the generation scripts. This approach means any TypeScript is valid in overrides (e.g. we can now use the & type operator).

I've tested this PR against the internal test suite and no additional changes were required for it to pass.

A copy of the generated types can be found here.

This PR also fixes a number of workers-types issues related to incorrect overrides:

Notably, #294 has not yet been fixed.

Closes cloudflare/workers-types#304.

@mrbbot mrbbot force-pushed the bcoll/types-overrides branch from 45fd54a to aad867d Compare October 31, 2022 18:00
@mrbbot mrbbot requested review from jasnell and penalosa October 31, 2022 18:05
@jasnell
Copy link
Member

jasnell commented Oct 31, 2022

Looks good to me. I'll hold off giving it the stamp tho as it would be good to give it some time for others to review.

Namely, it would be great to get some of the DO and R2 folks to review the overrides specific to those apis.

@xortive
Copy link
Contributor

xortive commented Nov 2, 2022

Some DO things (I reviewed the generated output linked in the PR description).

  • We should hide the ActorState type
  • DurableObjectStorageOperationsGetAlarmOptions should be called DurableObjectGetAlarmOptions. Same for SetAlarmOptions
  • setAlarm() is missing param names, and should accept number | Date for scheduledTime
  • Seems like several 1-param methods are missing param names but most of them seem to be "unnecessary" to figure out how to use the type. It's unfortunate to lose the nice names we have now and see param0 though, but I guess it's a lot of work to put overrides for all of them?
  • Looks like deleteAll() on DurableObjectTransaction should be never according to the override, but the generated type still has it defined?

@mrbbot mrbbot force-pushed the bcoll/types-overrides branch from aad867d to 2cc857a Compare November 3, 2022 11:37
@mrbbot
Copy link
Contributor Author

mrbbot commented Nov 3, 2022

@xortive

We should hide the ActorState type

DurableObjectStorageOperationsGetAlarmOptions should be called DurableObjectGetAlarmOptions. Same for SetAlarmOptions

...should accept number | Date for scheduledTime

Done ✅ Updated the generated definitions gist.

setAlarm() is missing param names

Seems like several 1-param methods are missing param names but most of them seem to be "unnecessary" to figure out how to use the type.

We're working on including parameter names in a separate PR. 👍

Looks like deleteAll() on DurableObjectTransaction should be never according to the override, but the generated type still has it defined?

Hmmm, I can only see one instance of deleteAll in DurableObjectStorage?

@mrbbot mrbbot force-pushed the bcoll/types-overrides branch 2 times, most recently from 5f1ddca to 57290f6 Compare November 8, 2022 15:27
@mrbbot
Copy link
Contributor Author

mrbbot commented Nov 8, 2022

@jasnell, @Frederik-Baetens approved the R2 bindings after pointing out R2HTTPMetdata was incorrectly named (now fixed). I've also applied @xortive's DO fixes, so I think we should be good to merge. 👍

@mrbbot mrbbot force-pushed the bcoll/types-overrides branch from 0b4773c to 46d8716 Compare November 9, 2022 12:23
@xortive xortive self-requested a review November 9, 2022 17:52
These allow the auto-generated TypeScript definitions to be
customised, leading to higher fidelity types.

- `JSG_(STRUCT_)TS_ROOT`: start visiting types for inclusion from
  this type
- `JSG_(STRUCT_)TS_OVERRIDE`: customise the generated definition for
  this type by merging or replacing it with a partial TypeScript
  definition
- `JSG_(STRUCT_)TS_DEFINE`: insert additional handwritten TypeScript
  next to the generated definition for this type
@mrbbot mrbbot force-pushed the bcoll/types-overrides branch from 46d8716 to 9229a5e Compare November 10, 2022 17:09
@Frederik-Baetens
Copy link
Contributor

Confirm that I've reviewed this on behalf of the R2 team. 👍 from me

@jasnell
Copy link
Member

jasnell commented Nov 11, 2022

@mrbbot ... I'll land this now. If there is a corresponding internal PR, please be sure to get that landed also :-)

@jasnell jasnell merged commit 8469338 into main Nov 11, 2022
@jasnell jasnell deleted the bcoll/types-overrides branch November 11, 2022 18:49
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.

Move overrides to workerd C++ source code
5 participants