-
-
Notifications
You must be signed in to change notification settings - Fork 12
feat: enable Ajv2019 and Ajv2020 #152
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?
Conversation
| this.ajv = new Ajv(Object.assign({}, defaultAjvOptions, options.customOptions)) | ||
| } | ||
| const ajvPath = ['JTD', '2019', '2020'].includes(options.mode) ? `ajv/dist/${options.mode.toLowerCase()}` : 'ajv' | ||
| const Ajv = require(ajvPath) |
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.
It doesn't look like same as before. Previously, accessing the .default.
| const Ajv = require(ajvPath) | |
| const Ajv = require(ajvPath) |
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.
This allows using Ajv2019 and Ajv2020 by passing 2019 or 2020 in options.mode.
If options.mode is not one of JTD, 2019, or 2020, ajvPath is "ajv" (the default).
If I'm misunderstanding the question, please help me understand.
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.
I means const Ajv = require('ajv').default, now becomes const Ajv = require('ajv')
Unsure why the .default exist.
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.
I see. Thanks for explaining. This became a learning opportunity for me.
What I learned is, Ajv is ESM. According to Node docs
When an ES Module contains both named exports and a default export, the result returned by require() is the module namespace object, which places the default export in the .default property, similar to the results returned by import(). To customize what should be returned by require(esm) directly, the ES Module can export the desired value using the string name "module.exports".
In Ajv code, Ajv uses module.exports, so require('ajv') should get the Ajv class, which is the default export. The JTD, 2019, and 2020 versions do the same.
My guess is that, in the past, require().default may have been required to get the default export out of the module namespace object. The JTD code wasn't using .default and worked and this code works without .default.
I don't mind adding a commit to add the .default if wanted.
This morning, I woke up thinking I should typeof options.mode === 'string' to the ternary condition to avoid trying to call toLowerCase on a non-string (would default to plain Ajv if options.mode is not a string or is not one of the three recognized options).
But Array.prototype.includes doesn't coerce types, so non-string won't match and won't attempt to call toLowerCase, so not needed. (Another thing I learned.)
|
Lint is failing in CI. When I ran lint locally on main, it failed with the same error. I'm not sure what the lint issue is. |
mcollina
left a comment
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.
lgtm
metcoder95
left a comment
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.
lint seems failing
| } else { | ||
| this.ajv = new Ajv(Object.assign({}, defaultAjvOptions, options.customOptions)) | ||
| } | ||
| const ajvPath = ['JTD', '2019', '2020'].includes(options.mode) ? `ajv/dist/${options.mode.toLowerCase()}` : 'ajv' |
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.
I would refactor a bit here, because it is strange to me that we don't support the default version.
I would implement something like:
const supportedModes = new Map()
supportedModes.add('2020', () => require('ajv/dist/2020'))
supportedModes.add('2019', () => require('ajv/dist/2019'))
supportedModes.add('draft-04', () => require('ajv/dist/refs/json-schema-draft-06.json'))
Co-authored-by: Manuel Spigolon <behemoth89@gmail.com> Signed-off-by: jmjf <jamee.mikell@gmail.com>
Lint fails in main (see also this comment) with the same errors. At the time I created this PR, I saw several merged PRs that had the same lint failures. If the team wants to recommend lint config changes the team considers acceptable, I'm willing to make them. (I use biome, not eslint, so am not familiar with eslint config.) |
Closes #151 and probably makes #105 moot.
The
for...ofapproach to tests seemed simple. If maintainers don't like it, I'm open to input on what you'd prefer.npm run testand(there is no benchmark script)npm run benchmarkand the Code of conduct
Checklist
npm run testandnpm run benchmarkand the Code of conduct