-
Notifications
You must be signed in to change notification settings - Fork 96
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.
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
Conversation
if (seen) { | ||
// short-circuit result if we've seen this object before | ||
return seen; | ||
} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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! 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really good! My half-baked attempt at solving the test is here: #246 But I wholly prefer your approach.
if (seen) { | ||
// short-circuit result if we've seen this object before | ||
return seen; | ||
} |
There was a problem hiding this comment.
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)
Published as part of https://github.com/blitz-js/superjson/releases/tag/v1.13.0 |
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [superjson](https://togithub.com/blitz-js/superjson) | [`1.12.4` -> `1.13.1`](https://renovatebot.com/diffs/npm/superjson/1.12.4/1.13.1) | [![age](https://developer.mend.io/api/mc/badges/age/npm/superjson/1.13.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/superjson/1.13.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/superjson/1.12.4/1.13.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/superjson/1.12.4/1.13.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>blitz-js/superjson (superjson)</summary> ### [`v1.13.1`](https://togithub.com/blitz-js/superjson/releases/tag/v1.13.1) [Compare Source](https://togithub.com/blitz-js/superjson/compare/v1.13.0...v1.13.1) #### What's Changed - Bump json5 from 1.0.1 to 1.0.2 by [@​dependabot](https://togithub.com/dependabot) in [https://github.com/blitz-js/superjson/pull/218](https://togithub.com/blitz-js/superjson/pull/218) - Bump minimatch from 3.0.4 to 3.1.2 by [@​dependabot](https://togithub.com/dependabot) in [https://github.com/blitz-js/superjson/pull/213](https://togithub.com/blitz-js/superjson/pull/213) - Bump decode-uri-component from 0.2.0 to 0.2.2 by [@​dependabot](https://togithub.com/dependabot) in [https://github.com/blitz-js/superjson/pull/212](https://togithub.com/blitz-js/superjson/pull/212) - Bump tough-cookie from 4.1.2 to 4.1.3 by [@​dependabot](https://togithub.com/dependabot) in [https://github.com/blitz-js/superjson/pull/248](https://togithub.com/blitz-js/superjson/pull/248) - fix: TypeError: SuperJSON is not a constructor by [@​juliusmarminge](https://togithub.com/juliusmarminge) in [https://github.com/blitz-js/superjson/pull/250](https://togithub.com/blitz-js/superjson/pull/250) #### New Contributors - [@​juliusmarminge](https://togithub.com/juliusmarminge) made their first contribution in [https://github.com/blitz-js/superjson/pull/250](https://togithub.com/blitz-js/superjson/pull/250) **Full Changelog**: flightcontrolhq/superjson@v1.13.0...v1.13.1 ### [`v1.13.0`](https://togithub.com/blitz-js/superjson/releases/tag/v1.13.0) [Compare Source](https://togithub.com/blitz-js/superjson/compare/v1.12.4...v1.13.0) #### What's Changed - fix: referential equalities should ignore child nodes by [@​KATT](https://togithub.com/KATT) in [https://github.com/blitz-js/superjson/pull/244](https://togithub.com/blitz-js/superjson/pull/244) - feat: add `dedupe` flag by [@​KATT](https://togithub.com/KATT) in [https://github.com/blitz-js/superjson/pull/247](https://togithub.com/blitz-js/superjson/pull/247) **Full Changelog**: flightcontrolhq/superjson@v1.12.4...v1.13.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/Chia1104/chia1104.dev). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi44LjExIiwidXBkYXRlZEluVmVyIjoiMzYuOC4xMSIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [superjson](https://togithub.com/blitz-js/superjson) | [`1.12.4` -> `1.13.1`](https://renovatebot.com/diffs/npm/superjson/1.12.4/1.13.1) | [![age](https://developer.mend.io/api/mc/badges/age/npm/superjson/1.13.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/superjson/1.13.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/superjson/1.12.4/1.13.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/superjson/1.12.4/1.13.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>blitz-js/superjson (superjson)</summary> ### [`v1.13.1`](https://togithub.com/blitz-js/superjson/releases/tag/v1.13.1) [Compare Source](https://togithub.com/blitz-js/superjson/compare/v1.13.0...v1.13.1) #### What's Changed - Bump json5 from 1.0.1 to 1.0.2 by [@​dependabot](https://togithub.com/dependabot) in [https://github.com/blitz-js/superjson/pull/218](https://togithub.com/blitz-js/superjson/pull/218) - Bump minimatch from 3.0.4 to 3.1.2 by [@​dependabot](https://togithub.com/dependabot) in [https://github.com/blitz-js/superjson/pull/213](https://togithub.com/blitz-js/superjson/pull/213) - Bump decode-uri-component from 0.2.0 to 0.2.2 by [@​dependabot](https://togithub.com/dependabot) in [https://github.com/blitz-js/superjson/pull/212](https://togithub.com/blitz-js/superjson/pull/212) - Bump tough-cookie from 4.1.2 to 4.1.3 by [@​dependabot](https://togithub.com/dependabot) in [https://github.com/blitz-js/superjson/pull/248](https://togithub.com/blitz-js/superjson/pull/248) - fix: TypeError: SuperJSON is not a constructor by [@​juliusmarminge](https://togithub.com/juliusmarminge) in [https://github.com/blitz-js/superjson/pull/250](https://togithub.com/blitz-js/superjson/pull/250) #### New Contributors - [@​juliusmarminge](https://togithub.com/juliusmarminge) made their first contribution in [https://github.com/blitz-js/superjson/pull/250](https://togithub.com/blitz-js/superjson/pull/250) **Full Changelog**: flightcontrolhq/superjson@v1.13.0...v1.13.1 ### [`v1.13.0`](https://togithub.com/blitz-js/superjson/releases/tag/v1.13.0) [Compare Source](https://togithub.com/blitz-js/superjson/compare/v1.12.4...v1.13.0) #### What's Changed - fix: referential equalities should ignore child nodes by [@​KATT](https://togithub.com/KATT) in [https://github.com/blitz-js/superjson/pull/244](https://togithub.com/blitz-js/superjson/pull/244) - feat: add `dedupe` flag by [@​KATT](https://togithub.com/KATT) in [https://github.com/blitz-js/superjson/pull/247](https://togithub.com/blitz-js/superjson/pull/247) **Full Changelog**: flightcontrolhq/superjson@v1.12.4...v1.13.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/Chia1104/chia-stack). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi4xMS4wIiwidXBkYXRlZEluVmVyIjoiMzYuMTEuMCIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->
Closes #245