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

Don't validate known lexicons at runtime #1790

Merged
merged 2 commits into from
Nov 1, 2023
Merged

Don't validate known lexicons at runtime #1790

merged 2 commits into from
Nov 1, 2023

Conversation

gaearon
Copy link
Contributor

@gaearon gaearon commented Nov 1, 2023

Here's what I'm seeing in the app's module initialization sequence with CPU throttling on:

Screenshot 2023-11-01 at 01 56 46

It looks like we're spending client's CPU cycles validating that our own schemas are valid. I'm not sure it makes sense for a client to do that in production. From what I understand, these schemas are known at the build time, there's no way for them to have changed by any other way than deploying our code. So if they were valid when the app was built, it seems unnecessary to check whether they're valid on every end user's computer.

Without these lines, the init time shrinks considerably:

Screenshot 2023-11-01 at 02 01 07

@pfrazee
Copy link
Collaborator

pfrazee commented Nov 1, 2023

I'm 100% on board with this

@dholms
Copy link
Collaborator

dholms commented Nov 1, 2023

I very much support something to this effect 👍

I'm actually curious if it makes sense to remove the check all together? I think this made it in when we thought lexicons might be getting loaded into the client much more dynamically. But the way things stand, you know all lexicons at build time. It may make sense down the line to allow dynamically adding a lexicon which would require a parsing check

We actually run these same typechecks on the lexicon documents during codegen, before they even have the chance to get build into the client. So it's not really serving us in DEV either

@devinivy
Copy link
Collaborator

devinivy commented Nov 1, 2023

What if the add() method here received a LexiconDoc rather than an unknown? Our built schemas are typed, so we should be ready to roll. We should just ensure lexicon doc parsing is exposed so that external docs can be validated and used as well.

export const schemas: LexiconDoc[] = Object.values(schemaDict) as LexiconDoc[]

@gaearon gaearon changed the title [Straw proposal] Make validation of schemas DEV-only Don't validate known lexicons at runtime Nov 1, 2023
@gaearon
Copy link
Contributor Author

gaearon commented Nov 1, 2023

Pushed changes:

  • Lexicons#constructor and Lexicons#add deal with LexiconDoc now.
    • Had to tweak a few types in tests for that.
  • Added top-level parseLexiconDoc(unknown): LexiconDoc as a throwing refinement.
    • Technically this could be done via explicit Zod lexiconDoc.parse() and a cast so we don't have to add it. I like that having this export suggests it's the supported way to validate those at runtime though so I think it's worth keeping.
    • Changed a few callsites that used Zod lexiconDoc.parse() to the new parseLexiconDoc() refinement.
    • This means we don't have to export Zod lexiconDoc anymore. I kept it around for consistency with how we export other Zod schemas.
  • I got rid of the special LexiconDocMalformedError error. It seems like it existed to highlight which specific schema failed but this isn't useful now that you do the validation yourself. Having direct access to Zod error without wrappers seems nicer — e.g. it lets us do error.format() which LexiconDocMalformedError didn't let us do. It doesn't seem like this library tries to abstract away Zod so let's just expose its raw error directly.

Hope this works!

@gaearon

This comment was marked as resolved.

@gaearon gaearon force-pushed the gaearon-patch-1 branch 2 times, most recently from 6ad0d0d to 4b3ebbf Compare November 1, 2023 04:23
@@ -415,16 +415,9 @@ export function isDiscriminatedObject(
return discriminatedObject.safeParse(value).success
}

export class LexiconDocMalformedError extends Error {
Copy link
Contributor Author

@gaearon gaearon Nov 1, 2023

Choose a reason for hiding this comment

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

Technically removing this would necessitate going to 0.3.0 on the off-chance that someone imports it. Not sure if we care about this at this point. Happy to re-add.

Copy link
Collaborator

@dholms dholms left a comment

Choose a reason for hiding this comment

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

biiigg yup 👍

thanks for pushing on this!

@gaearon gaearon merged commit cae5909 into main Nov 1, 2023
10 checks passed
@gaearon gaearon deleted the gaearon-patch-1 branch November 1, 2023 15:42
gaearon added a commit that referenced this pull request Nov 2, 2023
gaearon added a commit that referenced this pull request Nov 2, 2023
@Olegbobokhidze
Copy link

❤️

mloar pushed a commit to mloar/atproto that referenced this pull request Nov 15, 2023
* Make lexicon validation DEV-only

* Apply code review suggestions
mloar pushed a commit to mloar/atproto that referenced this pull request Nov 15, 2023
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.

5 participants