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

feat: compile ajv validators at build instead of runtime #134

Merged
merged 10 commits into from
Aug 29, 2022

Conversation

cmcewen
Copy link
Contributor

@cmcewen cmcewen commented Aug 10, 2022

fixes #51

@vercel
Copy link

vercel bot commented Aug 10, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
widgets ✅ Ready (Inspect) Visit Preview Aug 18, 2022 at 4:01PM (UTC)

@vercel vercel bot temporarily deployed to Preview August 10, 2022 20:15 Inactive
@vercel vercel bot temporarily deployed to Preview August 10, 2022 20:25 Inactive
@zzmp zzmp self-requested a review August 15, 2022 22:26
@@ -0,0 +1,20 @@
/* eslint-disable */
Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid a new pattern, can this be moved to a top-level file called ajv.config.js?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so that eslint won't check it? idk i think that would be a little bit of misnaming because it's not a config file

Copy link
Contributor

Choose a reason for hiding this comment

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

not for eslint, just to avoid more complexity in the directory tree by adding a new scripts folder. What about just ajv.js?

const addFormats = require('ajv-formats')
const schema = require('@uniswap/token-lists/dist/tokenlist.schema.json')

const ajv = new Ajv({ code: { source: true, esm: true } })
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not write both schemas to one file (see docs)

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 was fighting with it because the schemas have the same name, so it didn't like using the same instance. imo not worth the additional time to make it work

src/hooks/useTokenList/validateTokenList.ts Outdated Show resolved Hide resolved
/**
* Validates an array of tokens.
* @param json the TokenInfo[] to validate
*/
export async function validateTokens(json: TokenInfo[]): Promise<TokenInfo[]> {
const validate = (await validator).getSchema(ValidationSchema.TOKENS)
const validate = await loadValidator(ValidationSchema.TOKENS)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you consider a single validate(schema: ValidationSchema, data: unknown) function?

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 changed it, but i can't say i think it's much better - you lose type specificity and it doesn't save much in terms of LOC


# generated ajv schema
/src/__generated__/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless you feel strongly about it, I'd rather this be put in something like /src/ajv or /src/schema or /src/validators, that better describes what it is. This is what is done with /src/types/v3 and /src/abis/types (at the top of the file). I also don't want a separate pattern here for file generation.

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 do feel like it is useful to know which code is generated vs authored. maybe a "lib" folder is better for that? the more practical reason is for lint/type ignores imo, but also i think you don't necessarily want to read generated code

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'd be willing to move all of that code to a generated file path or lib if you agree

Copy link
Contributor

Choose a reason for hiding this comment

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

Would you be willing to put this into /src/validators, and then follow-up by moving all generated code to a __generated directory?

@vercel vercel bot temporarily deployed to Preview August 17, 2022 21:57 Inactive
@vercel vercel bot temporarily deployed to Preview August 18, 2022 15:30 Inactive
@vercel vercel bot temporarily deployed to Preview August 18, 2022 16:01 Inactive
Copy link
Contributor

@zzmp zzmp left a comment

Choose a reason for hiding this comment

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

Could be cleaned up by making a helper function instead of ajv and ajv2, and changing the file structure, but this is definitely good to merge and be cleaned up later.

@cmcewen cmcewen merged commit 4625893 into main Aug 29, 2022
JFrankfurt added a commit that referenced this pull request Aug 30, 2022
JFrankfurt added a commit that referenced this pull request Aug 30, 2022
#134)" (#179)

Revert "feat: compile ajv validators at build instead of runtime (#134)"

This reverts commit 4625893.
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.

Unable to use Widget without unsafe-eval CSP
3 participants