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

avoid recursion when packing with toJSON #117

Merged
merged 1 commit into from
Jun 6, 2023

Conversation

paul-marechal
Copy link
Contributor

For reasons outside of my control, someone might return this when implementing toJSON causing the packing logic to go into an infinite loop.

This commit implements a guard against this specific case. The logic will continue through if value.toJSON() === value.

For reasons outside of my control, someone might return `this` when
implementing `toJSON` causing the packing logic to go into an infinite
loop.

This commit implements a guard against this specific case. The logic
will continue through if `value.toJSON() === value`.
@paul-marechal
Copy link
Contributor Author

It would greatly help if this was released as a patch release.

Copy link
Owner

@kriszyp kriszyp left a comment

Choose a reason for hiding this comment

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

I wonder if adding this toJSON might have been a mistake. I will do this for patch, but maybe remove for 1.10.

@kriszyp kriszyp merged commit 524ebd9 into kriszyp:master Jun 6, 2023
@kriszyp
Copy link
Owner

kriszyp commented Jun 6, 2023

Thank you for the PR! Published in 1.9.5.

@paul-marechal paul-marechal deleted the avoid-pack-recursion branch June 6, 2023 23:37
@paul-marechal
Copy link
Contributor Author

I wonder if adding this toJSON might have been a mistake.

I don't have super useful feedback other than that I don't know why it was introduced, and the issue fixed by this PR isn't really a showstopper but rather what feels like some odd corner case.

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.

2 participants