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.

Sign up for GitHub

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

Jump to bottom

Replace Runtypes with Zod in starter templates #984

Merged
merged 11 commits into from
Oct 11, 2022
Merged

Replace Runtypes with Zod in starter templates #984

merged 11 commits into from
Oct 11, 2022

Conversation

JasonZzy0528
Copy link
Contributor

@JasonZzy0528 JasonZzy0528 commented Oct 6, 2022 •
edited by 72636c

Proposed Changes

Closes #983.

High level summary of change

Replace Runtypes with Zod in templates:

  • koa-rest-api
  • lambda-sqs-worker
  • lambda-sqs-worker-cdk

@changeset-bot
Copy link

changeset-bot bot commented Oct 6, 2022 •
edited

🦋 Changeset detected

Latest commit: 3425216

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
skuba Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@JasonZzy0528 JasonZzy0528 marked this pull request as ready for review October 6, 2022 04:34
@JasonZzy0528 JasonZzy0528 requested review from a team as code owners October 6, 2022 04:34
@JasonZzy0528 JasonZzy0528 changed the title #983 Replace Runtypes with Zod in starter templates Replace Runtypes with Zod in starter templates Oct 6, 2022
@samchungy
Copy link
Contributor

WEW :)

samchungy
samchungy reviewed Oct 6, 2022
View reviewed changes
Copy link
Contributor

@samchungy samchungy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is some seriously good work 🎉 Just a few nitpicks

template/koa-rest-api/package.json Outdated Show resolved Hide resolved
template/lambda-sqs-worker-cdk/package.json Outdated Show resolved Hide resolved
template/lambda-sqs-worker/package.json Outdated Show resolved Hide resolved
template/lambda-sqs-worker-cdk/shared/context-types.ts Outdated Show resolved Hide resolved
@JasonZzy0528 JasonZzy0528 force-pushed the 983-replace-runtypes-with-zod branch 2 times, most recently from e0a4ac6 to 6d4d61f Compare October 10, 2022 03:28
brian-captain-crosby
brian-captain-crosby approved these changes Oct 10, 2022
View reviewed changes
Copy link
Contributor

@brian-captain-crosby brian-captain-crosby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome 👏

72636c
72636c reviewed Oct 10, 2022
View reviewed changes
Copy link
Member

@72636c 72636c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution!

Can we also look at enforcing .strict() where applicable? For example, it may make sense when the API template parses a request, as we don't want clients sending random junk fields that may conflict with future extensions. OTOH the Lambda template should only be concerned with extracting the subset of the external job scorer response that it needs, so non-strict parsing is suitable there.

template/lambda-sqs-worker-cdk/shared/context-types.ts Outdated Show resolved Hide resolved
template/koa-rest-api/src/framework/validation.test.ts Outdated Show resolved Hide resolved
template/koa-rest-api/src/api/jobs/postJob.test.ts Show resolved Hide resolved
@JasonZzy0528
Copy link
Contributor Author

JasonZzy0528 commented Oct 11, 2022 •
edited

The Integration should be b̵r̵o̵k̵e̵n̵ ̵d̵u̵e̵ ̵t̵o̵ ̵t̵h̵e̵ ̵l̵a̵t̵e̵s̵t̵ ̵v̵e̵r̵s̵i̵o̵n̵(̵̵1̵6̵.̵1̵1̵.̵6̵5̵̵)̵ ̵o̵f̵ ̵@̵t̵y̵p̵e̵s̵/̵n̵o̵d̵e̵ passed after rebased from the master branch.

72636c
72636c approved these changes Oct 11, 2022
View reviewed changes
Copy link
Member

@72636c 72636c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wonderful, thanks Jason!

I'd like us to examine .strict() in more detail but that can be a separate PR 🙂

@72636c 72636c enabled auto-merge (squash) October 11, 2022 03:39
@72636c 72636c merged commit 1753391 into seek-oss:master Oct 11, 2022
@seek-oss-ci seek-oss-ci mentioned this pull request Oct 11, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samchungy samchungy samchungy left review comments

@brian-captain-crosby brian-captain-crosby brian-captain-crosby approved these changes

@72636c 72636c 72636c approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Replace Runtypes with Zod in starter templates
4 participants
@JasonZzy0528 @samchungy @brian-captain-crosby @72636c