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

feat: add dedupe flag #247

Merged
merged 15 commits into from
Jul 17, 2023
+64 −5
Merged

feat: add dedupe flag #247

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
15 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
cb35784
feat: add dedupe flag
KATT Jul 14, 2023
c18c4e6
feat: add dedupe flag
KATT Jul 14, 2023
abf87c8
rm useless test
KATT Jul 14, 2023
dc0e4b0
refactor: name dedupeReferentialEqualities
Skn0tt Jul 17, 2023
cacdd60
fix: dedupe is enough
Skn0tt Jul 17, 2023
466de68
fix: test
Skn0tt Jul 17, 2023
308d15b
fix: test
Skn0tt Jul 17, 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
55 changes: 55 additions & 0 deletions src/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1157,3 +1157,58 @@ test('regression #245: superjson referential equalities only use the top-most pa
const parsed = SuperJSON.deserialize(res);
expect(parsed).toEqual(input);
});

test('dedupe=true', () => {
{
const instance = new SuperJSON();
instance.dedupe = true;

type Node = {
children: Node[];
};
const root: Node = {
children: [],
};
const input = {
a: root,
b: root,
};
const output = instance.serialize(input);

const json = output.json as any;

expect(json.a);

// This has already been seen and should be deduped
expect(json.b).toBeNull();

expect(json).toMatchInlineSnapshot(`
Object {
"a": Object {
"children": Array [],
},
"b": null,
}
`);

expect(instance.deserialize(output)).toEqual(input);
}

{
KATT marked this conversation as resolved.
Show resolved Hide resolved
const instance = new SuperJSON();
type Node = {
children: Node[];
};
const root: Node = {
children: [],
};
const input = {
a: root,
b: root,
};
const output = instance.serialize(input);

// Ensure that we have no breaking changes
expect(output.json).toEqual(input);
}
});
5 changes: 5 additions & 0 deletions src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@ import {
import { copy } from 'copy-anything';

export default class SuperJSON {
/**
* If true, SuperJSON will make sure only one instance of referentially equal objects are serialized and the rest are replaced with `null`.
*/
public dedupe = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

My gut instinct was to put this as an optional parameter to serialize, and call it something like pruneReferentialEqualities, in reference to meta.referentialEqualities. I'd also be fine with dedupeReferentialEqualities. It's a lot longer of a name, but more descriptive I guess? But also harder to parse, so i'm torn between the two. Any strong feelings from your end on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding an optional argument to serialize could be seen as a breaking change in cases like items.map(superjson.serialize.bind(superjson) 🙃

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see where you're coming from, but I don't necessarily agree with that strict of a definition of "breaking change" 😅


serialize(object: SuperJSONValue): SuperJSONResult {
const identities = new Map<any, any[][]>();
const output = walker(object, identities, this);
Expand Down
6 changes: 5 additions & 1 deletion src/plainer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,11 @@ export const walker = (
const seen = seenObjects.get(object);
if (seen) {
// short-circuit result if we've seen this object before
return seen;
return superJson.dedupe
? {
transformedValue: null,
}
Comment on lines +171 to +173
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It'd be nice with some symbol here (and for circular references fwiw) to show in the JSON that we have replaced it

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree. I wonder if there's some other implementation that we can draw inspiration from 🤔

Apparently Node.js prints [Circular *1]:
Screenshot 2023-07-14 at 16 34 28

Same with Deno. This might even be a v8 thing.

Screenshot 2023-07-14 at 16 44 50

What if we did [Circular] and [Pruned]?

: seen;
}
}

Expand Down