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

WIP - Add endpoint validation #671

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

tompretty
Copy link
Contributor

What does this change?

Add validation to the data endpoints.

How to test

How can we measure success?

Have we considered potential risks?

Images

Accessibility

@@ -1,18 +1,30 @@
import { PageTracking } from './shared';
import { PageTrackingSchema } from './shared';
import { z } from 'zod';
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately we cannot add zod to files in /targeting because they get used by the dotcom package.
I need to find a way to prevent this as it's not at all obvious!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh yeah I thought this might be the case - is there some way to confirm this is happening? I tried building on the main branch and then on this one and couldn't see a difference in bundle size

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the solution here might be to add another package under /packages e.g validation that does all of the zod stuff which will keep it clear of anything the dotcom package uses

Copy link
Member

Choose a reason for hiding this comment

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

That's a good suggestion!
So far I've just looked in the generated js in /packages/dotcom/dist/ to make sure it's not bringing in unwanted code, e.g. zod. But this isn't really acceptable!

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.

2 participants