-
Notifications
You must be signed in to change notification settings - Fork 90
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: insert "symbols" instead of null
for deduped objects
#251
base: main
Are you sure you want to change the base?
fix: insert "symbols" instead of null
for deduped objects
#251
Conversation
"[Circular *]", | ||
], | ||
}, | ||
"b": "[Pruned *]", |
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.
should this say [Pruned <path-where-the-node-is>]
or something?
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.
Since we're adding this for the output to be more human-readable, I think we should take a string that's searchable + self-explanatory. I don't think Pruned
cuts it. What do you think about something like[SuperJSON: Equal to <path.where.node>]
?
Co-authored-by: Alex / KATT <[email protected]>
@@ -195,7 +195,7 @@ export const walker = ( | |||
if (includes(objectsInThisPath, object)) { | |||
// prevent circular references | |||
return { | |||
transformedValue: null, | |||
transformedValue: '[Circular *]', |
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.
Let's also put the path to the object that's circular into here.
I don't regard the specific structure of the outputted JSON as being part of SuperJSON's API contract, so I don't think this would be a breaking change. I'd release this as a minor release. |
Excuse the late reply btw! My understanding is that this PR isn't urgent for your usecases, so I haven't prioritise this until now (I'm a bit under-water currently). If you need this more urgently, let me know and i'll try to prioritise it earlier. |
Resolves #247 (comment)
Not sure how you wanna do regarding semver?