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.

Sign up for GitHub

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

fix: referential equalities should ignore child nodes #244

Merged
merged 8 commits into from
Jul 14, 2023
Merged

fix: referential equalities should ignore child nodes #244

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0d95739
add failing test
KATT Jul 14, 2023
b3f907f
add failing test
KATT Jul 14, 2023
2b4dbe6
tweak
KATT Jul 14, 2023
7b0742d
fix test
KATT Jul 14, 2023
b1fb75a
Update src/plainer.ts
KATT Jul 14, 2023
86f0295
Update src/plainer.ts
KATT Jul 14, 2023
1d134ee
less diff
KATT Jul 14, 2023
ea9f0b0
Merge branch 'regression' of github.com:KATT/superjson into regression
KATT Jul 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions src/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1126,3 +1126,34 @@ test('superjson instances are independent of one another', () => {
const res2 = s2.serialize(value);
expect(res2.json).toEqual(value);
});

test('regression #245: superjson referential equalities only use the top-most parent node', () => {
type Node = {
children: Node[];
};
const root: Node = {
children: [],
};
const input = {
a: root,
b: root,
};
const res = SuperJSON.serialize(input);

expect(res.meta?.referentialEqualities).toHaveProperty(['a']);

// saying that a.children is equal to b.children is redundant since its already know that a === b
expect(res.meta?.referentialEqualities).not.toHaveProperty(['a.children']);
expect(res.meta).toMatchInlineSnapshot(`
Object {
"referentialEqualities": Object {
"a": Array [
"b",
],
},
}
`);

const parsed = SuperJSON.deserialize(res);
expect(parsed).toEqual(input);
});
73 changes: 43 additions & 30 deletions src/plainer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,27 +155,40 @@ export const walker = (
identities: Map<any, any[][]>,
superJson: SuperJSON,
path: any[] = [],
objectsInThisPath: any[] = []
objectsInThisPath: any[] = [],
seenObjects = new Map<unknown, Result>()
): Result => {
if (!isPrimitive(object)) {
const primitive = isPrimitive(object);

if (!primitive) {
addIdentity(object, path, identities);

const seen = seenObjects.get(object);
if (seen) {
// short-circuit result if we've seen this object before
return seen;
}
Comment on lines +167 to +170
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 actually would prefer to return the same here as when we hit circular references here ({ transformedValue: null }) as it would make the output a lot smaller, but that would likely be a breaking change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Skn0tt:
would it be OK to return null here under some sort of options flag? it would make big outputs a lot smaller

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like that idea conceptually - less duplicated data is always better - but I agree it's both a breaking change (the output JSON wouldn't have the same structure anymore), and it makes SuperJSON output harder to understand.

I'm assuming you'd like to reduce the output size to save bandwidth, right? Maybe we could perform some kind of benchmark on the difference this would make in practice - I could imagine that with Gzip / Brotli, the difference isn't that big. If we find that the difference is significant even with compression, then I agree we should add a flag for this behaviour! (not in this PR though)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do a follow-up PR! 👀

}

if (!isDeep(object, superJson)) {
const transformed = transformValue(object, superJson);
if (transformed) {
return {
transformedValue: transformed.value,
annotations: [transformed.type],
};
} else {
return {
transformedValue: object,
};

const result: Result = transformed
? {
transformedValue: transformed.value,
annotations: [transformed.type],
}
: {
transformedValue: object,
};
if (!primitive) {
seenObjects.set(object, result);
}
return result;
}

if (includes(objectsInThisPath, object)) {
// prevent circular references
return {
transformedValue: null,
};
Expand All @@ -184,10 +197,6 @@ export const walker = (
const transformationResult = transformValue(object, superJson);
const transformed = transformationResult?.value ?? object;

if (!isPrimitive(object)) {
objectsInThisPath = [...objectsInThisPath, object];
}

const transformedValue: any = isArray(transformed) ? [] : {};
const innerAnnotations: Record<string, Tree<TypeAnnotation>> = {};

Expand All @@ -197,7 +206,8 @@ export const walker = (
identities,
superJson,
[...path, index],
objectsInThisPath
[...objectsInThisPath, object],
seenObjects
);

transformedValue[index] = recursiveResult.transformedValue;
Expand All @@ -211,19 +221,22 @@ export const walker = (
}
});

if (isEmptyObject(innerAnnotations)) {
return {
transformedValue,
annotations: !!transformationResult
? [transformationResult.type]
: undefined,
};
} else {
return {
transformedValue,
annotations: !!transformationResult
? [transformationResult.type, innerAnnotations]
: innerAnnotations,
};
const result: Result = isEmptyObject(innerAnnotations)
? {
transformedValue,
annotations: !!transformationResult
? [transformationResult.type]
: undefined,
}
: {
transformedValue,
annotations: !!transformationResult
? [transformationResult.type, innerAnnotations]
: innerAnnotations,
};
if (!primitive) {
seenObjects.set(object, result);
}

return result;
};