Skip to content

Commit

Permalink
Don't add undefined values to objects when parsing JSON (#2127)
Browse files Browse the repository at this point in the history
## Summary:
Previously, when parsing typed objects from JSON, we would add
`[key]: undefined` for any fields that were specified in the parser schema but
were absent from the JSON. This was a difference from the behavior of
`JSON.parse()` that could cause bugs in some cases (e.g. if a parsed
object was spread into another object, it could overwrite existing
values with `undefined` when that was not intended).

Thus, to better match the behavior of `JSON.parse()`, we now omit
`undefined` values from the parsed object if they are not present on the
original object.

This PR was inspired by test failures on Khan/webapp#28551.
I'm making this change out of an abundance of caution.

Issue: LEMS-2774

Test plan:

`yarn test`

Author: benchristel

Reviewers: jeremywiebe, handeyeco

Required Reviewers:

Approved By: jeremywiebe

Checks: ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x)

Pull Request URL: #2127
  • Loading branch information
benchristel authored Jan 18, 2025
1 parent 518b005 commit 6f2813c
Show file tree
Hide file tree
Showing 4 changed files with 26 additions and 1,423 deletions.
5 changes: 5 additions & 0 deletions .changeset/happy-pets-breathe.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/perseus": minor
---

Avoid adding undefined values to objects parsed from Perseus JSON when properties are missing.
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import {assertFailure, success} from "../result";
import {assertFailure, assertSuccess, success} from "../result";

import {array} from "./array";
import {defaulted} from "./defaulted";
import {number} from "./number";
import {object} from "./object";
import {optional} from "./optional";
import {string} from "./string";
import {anyFailure, ctx, parseFailureWith} from "./test-helpers";

Expand Down Expand Up @@ -93,4 +94,19 @@ describe("object", () => {

expect(Train({}, ctx())).toEqual(success({boxcars: []}));
});

it("does not include fields not present on the original object", () => {
const Penguin = object({hat: optional(string)});
const result = Penguin({}, ctx());
assertSuccess(result);
expect(result.value).not.toHaveProperty("hat");
});

it("includes `undefined` fields from the original object", () => {
const Penguin = object({hat: optional(string)});
const result = Penguin({hat: undefined}, ctx());
assertSuccess(result);
expect(result.value).toHaveProperty("hat");
expect(result.value.hat).toBe(undefined);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@ export function object<S extends ObjectSchema>(
for (const [prop, propParser] of Object.entries(schema)) {
const result = propParser(rawValue[prop], ctx.forSubtree(prop));
if (isSuccess(result)) {
ret[prop] = result.value;
if (result.value !== undefined || prop in rawValue) {
ret[prop] = result.value;
}
} else {
mismatches.push(...result.detail);
}
Expand Down
Loading

0 comments on commit 6f2813c

Please sign in to comment.