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.

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

juliusmarminge
Copy link
Contributor

@juliusmarminge juliusmarminge commented Jul 17, 2023

Resolves #247 (comment)

Not sure how you wanna do regarding semver?

@juliusmarminge juliusmarminge requested a review from Skn0tt as a code owner July 17, 2023 11:28
"[Circular *]",
],
},
"b": "[Pruned *]",
Copy link
Contributor Author

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?

Copy link
Collaborator

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>]?

src/index.test.ts Outdated Show resolved Hide resolved
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 *]',
Copy link
Collaborator

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.

@Skn0tt
Copy link
Collaborator

Skn0tt commented Jul 27, 2023

Not sure how you wanna do regarding semver?

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.

@Skn0tt
Copy link
Collaborator

Skn0tt commented Jul 27, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants