Repository navigation
feat: include the expected value in const errors - #326
Dhairya-007 wants to merge 5 commits into
Conversation
ajv reports const failures as "must be equal to constant" with the expected value in params.allowedValue, which was dropped. Mirrors the enum error, which already appends its allowed values.
Covers the js and cli formats, and non-string constants.
🦋 Changeset detectedLatest commit: 177154d The changes in this PR will be included in the next version bump. 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 |
|
Hooray! All contributors have signed the CLA. |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #326 +/- ##
==========================================
+ Coverage 90.78% 98.41% +7.63%
==========================================
Files 12 15 +3
Lines 217 316 +99
Branches 69 81 +12
==========================================
+ Hits 197 311 +114
+ Misses 19 5 -14
+ Partials 1 0 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
torifat
left a comment
There was a problem hiding this comment.
Thanks for this. The oneOf case in helpers.ts is the main one, the rest is small.
| return ret.concat( | ||
| new RequiredValidationError(error as AjvError<RequiredParams>, options) | ||
| ); | ||
| case 'const': |
There was a problem hiding this comment.
const skips the enum handling in filterRedundantErrors and the merge above, which breaks oneOf unions. With
oneOf: [
{ properties: { kind: { const: 'a' }, x: { type: 'number' } } },
{ properties: { kind: { const: 'b' }, y: { type: 'string' } } },
]and { kind: 'a', x: 'nope' }, we now print /kind must be equal to constant: "b". That tells the user to change the one value that's right. enum: ['b'] there gets filtered out. Two anyOf branches with const: 'a' and const: 'b' also give two errors on /kind instead of one.
Could const be handled as a one-value enum, allowedValues: [params.allowedValue]? That fixes both and adds the "Did you mean" suggestion.
There was a problem hiding this comment.
Done , const is now rewritten to { keyword: 'enum', params: { allowedValues: [allowedValue] } } before the tree is built, so it goes through the enum path. Your example now gives /x: type must be number plus the oneOf error, with no /kind error, and the two anyOf branches collapse into one.
|
|
||
| // The allowed value is printed as JSON so a string is quoted and tells the reader | ||
| // that the schema expects exactly that text. | ||
| const toText = (value: unknown): string => JSON.stringify(value) ?? String(value); |
There was a problem hiding this comment.
JSON.stringify throws on a circular object or a BigInt, so betterAjvErrors would now throw where it used to return errors. It also prints NaN and Infinity as null. A try/catch falling back to String(value) would cover the throw.
Also, enum's toText uses String, so enum: ['a'] prints a and const: 'a' prints "a". I'd keep them the same.
There was a problem hiding this comment.
Resolved by removing the file. The shared enum rendering uses String(), so circular objects, BigInt, NaN and Infinity no longer throw, and const matches enum by construction rather than by agreement.
|
|
||
| const output = [ | ||
| styleText('red', styleText('bold', 'CONST') + ' ' + message), | ||
| styleText('red', `(${toText(allowedValue)})\n`), |
There was a problem hiding this comment.
A large object constant prints in full here and again in the pointer line. Maybe truncate it past some length?
There was a problem hiding this comment.
Nothing is truncated, but nothing long is printed either: String() gives [object Object] for an object and 1,2,3 for an array. The length problem is gone, at the cost of an object constant no longer showing its contents. That matches enum: [someObject] today. If you'd rather keep the detail, I'll add a shared toText on BaseValidationError that stringifies with a try/catch and truncates, used by both.
| "better-ajv-errors": minor | ||
| --- | ||
|
|
||
| Name the expected value in `const` errors. ajv reports a `const` failure as `must be equal to constant` and carries the expected value in `params.allowedValue`, which was dropped because `const` had no handler of its own. A `const` error now reads `/node must be equal to constant: ">=18.12.0"`, with the value printed as JSON so strings stay quoted and `null`, `false` and numbers are unambiguous. The `cli` format gains the value in its header and in the codeframe pointer. |
There was a problem hiding this comment.
The cli header doesn't change, it's still CONST must be equal to constant. The value goes on the next line and in the pointer.
There was a problem hiding this comment.
Reworded to describe the enum rewrite and drop the header claim.
| ]); | ||
| }); | ||
|
|
||
| describe('const', () => { |
There was a problem hiding this comment.
Can you add a oneOf case like the one in helpers.ts, and a circular constant once toText handles it?
There was a problem hiding this comment.
Added: a oneOf case asserting no /kind error, the circular constant, and a parity test that const and enum give the same paths and counts for both the oneOf and anyOf shapes.
Dropping ConstValidationError and rewriting the ajv error to an enum with one allowed value means const goes through the enum path instead of around it, so oneOf branches that don't match no longer get reported and sibling branches merge into one error. Also gets the did-you-mean suggestion for free, and drops the JSON.stringify that threw on circular values.
|
Thanks, the Your example now gives:
Two things worth your call:
The changeset is reworded — you were right that the cli header never changed. |
|
One more on your truncation point, which I glossed over above. Nothing is truncated, but nothing long is printed either: the shared enum rendering uses That is exactly what |
|
Hii @torifat , good for re-review now |
Closes #162.
The issue asked whether the expected constant could be included in the message. ajv already hands it to us in
params.allowedValue; it was just being dropped, becauseconsthad no handler and fell through toDefaultValidationError.Before / after
Using the exact schema from the issue:
CLI format:
Approach
A
ConstValidationErrorclass, modelled onEnumValidationError— which already appends its allowed values in the same shape — rather than special-casingconstinside the default handler. So: newConstParamstype, the class, the barrel export, and onecase 'const':increateErrorInstances.The expected value is printed with
JSON.stringify, notString. For a single value that's worth it: it keeps">=18.12.0"distinguishable from a bare token, andnull,falseand8080print unambiguously.EnumValidationErrorusesStringbecause it joins a list, where quoting every item would be noisy — happy to match it instead if you'd rather have them identical.Tests
Three integration tests in
src/__tests__/index.ts(real ajv, thentoEqualon thejsoutput — matching the newer tests in that file rather than adding fixtures and snapshots):8080,null,falseclioutput contains the value, both in the header and the codeframe pointerWith the
case 'const':routing reverted, all three fail with the old"/node: const must be equal to constant", so they're pinning the actual behaviour.No existing test or snapshot covered
const, so nothing else moved.Checks
vp test84/84,format:checkclean,lint(vp lint+publint) clean.Note: run on Node 22. On Node 20 the
buildstep (and solint) fails withPromise.withResolvers is not a functioninsidevite-plus, which also happens on a cleanmain— unrelated to this PR.