Add tests to ensure Perseus JSON parsers are idempotent #2403
Conversation
|
Size Change: +14 B (0%) Total Size: 466 kB
ℹ️ View Unchanged
|
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (d36f830) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR2403If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.js -t PR2403 |
benchristel
force-pushed
the
benc/mutual-assignability
branch
from
1f59a66 to
e710c9c
Compare
benchristel
force-pushed
the
benc/idempotent-defaulted
branch
from
7cf70d6 to
bcb6197
Compare
| parser: Parser<T>, | ||
| fallback: (missingValue: null | undefined) => Default, | ||
| ): Parser<T | Default> { | ||
| fallback: (missingValue: null | undefined) => NoInfer<T>, |
There was a problem hiding this comment.
TIL about NoInfer<T> (I read https://www.totaltypescript.com/noinfer).
So this blocks the fallback's return value from being included in the type inference for T, right? So the return value's T will be only inferred from the parser type.
There was a problem hiding this comment.
Correct! FWIW writing this code made me happy because I think it's a perfect place to use NoInfer.
| import {defaulted} from "../general-purpose-parsers/defaulted"; | ||
|
|
||
| import {parseImages} from "./images-map"; | ||
| import {parseWidgetsMap} from "./widgets-map"; | ||
|
|
||
| export const parseHint = object({ | ||
| replace: defaulted(boolean, () => undefined), | ||
| replace: defaulted(optional(boolean), () => undefined), |
There was a problem hiding this comment.
I'm not clear why you needed to wrap optional() here? Is it because the replace can be undefined and if we didn't wrap it in optional() here, we'd have a resulting type of boolean because the fallback is ignored for type inference?
| // once is the same as running it many times. Idempotency is | ||
| // valuable because it means e.g. that if we run the parser on data | ||
| // before saving it to datastore, it won't be changed by being |
…nsure Perseus JSON parsers are idempotent
benchristel
force-pushed
the
benc/idempotent-defaulted
branch
from
bcb6197 to
d36f830
Compare
Summary:
I realized that the type of the
defaultedparser encouraged non-idempotentparsing. Essentially, a default value could be supplied that would cause a parse
error on a second pass through the parser. Example:
The inferred type
ParsedValue<typeof parseMyObject>would be:However, the parser doesn't consider a myField value of type
stringto be valid!This contradicts the types! You will get an error if you try to parse
because the parser for
myFieldonly accepts a number, null, or undefined asinput.
This is clearly not how we want parsers to behave. Parsing should be idempotent.
This PR fixes the types of
defaultedso we don't encourage non-idempotentparsing. It also adds tests to verify that our existing parsers are idempotent
when run on example data.
Issue: LEMS-3055
Test plan:
pnpm test