-
Notifications
You must be signed in to change notification settings - Fork 44
codegen cleanup #1101
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
base: main
Are you sure you want to change the base?
codegen cleanup #1101
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 4cf3e63:
|
7de34b2 to
4cf3e63
Compare
| "ACTIVITY_TYPE_CREATE_READ_WRITE_SESSION_V2", | ||
| ACTIVITY_TYPE_UPDATE_POLICY: "ACTIVITY_TYPE_UPDATE_POLICY_V2", | ||
| ACTIVITY_TYPE_INIT_OTP_AUTH: "ACTIVITY_TYPE_INIT_OTP_AUTH_V2", | ||
| ACTIVITY_TYPE_INIT_OTP: "ACTIVITY_TYPE_INIT_OTP", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
preparation; smart
| fs.writeFileSync(outputPath, output); | ||
|
|
||
| // after successfully generating the consolidated types, remove the | ||
| // input `public_api.types.ts` since it's no longer needed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could be argument to be made that we still include it here; otherwise, this script is no longer idempotent and repeated runs won't yield the same output (given the file won't be there anymore)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair point - I plan on moving our code generation scripts to a shared location (e.g. sdk/scripts), so we only need to maintain the swagger.json and the related types in one place
for now, I’m indifferent about including this to keep things idempotent. One argument for removing it is that it’s a fairly large file, so it does add a noticeable amount to the package size
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TLDR: if you want to add this back we can
Summary & Motivation
public_api_types.tsfromcoreandsdk-typessince it's not needed- for
sdk-typeswe use it during codegen, after we delete it- for
corewe just never get it anymoreACTIVITY_TYPE_INIT_OTPto ourVERSIONED_ACTIVITY_TYPESHow I Tested These Changes
Did you add a changeset?
If updating one of our packages, you'll likely need to add a changeset to your PR. To do so, run
pnpm changeset.pnpm changesetwill generate a file where you should write a human friendly message about the changes. Note how this (example) includes the package name (should be auto added by the command) along with the type of semver change (major.minor.patch) (which you should set).These changes will be used at release time to determine what packages to publish and how to bump their version. For more context see this comment.