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

Collisions #3

Closed
zloirock opened this issue Oct 14, 2024 · 2 comments
Closed

Collisions #3

zloirock opened this issue Oct 14, 2024 · 2 comments

Comments

@zloirock
Copy link

When I wrote a polyfill for this feature, I considered many different options, including something similar to used in your ponyfill. I already don't remember all the pitfalls I encountered and why I chose full parsing for parse. However, your code has at least one obvious problem:

parse('{"a":9007199254740993,"b":9007199254740992}', reviver); // => { a: 9007199254740992, b: 9007199254740992 }
parse('{"a":9007199254740992,"b":9007199254740993}', reviver); // => { a: 9007199254740993n, b: 9007199254740993n }
@WebReflection
Copy link
Member

I see, yes I need to change numbers as key into special strings as key so that each number will preserve its identity, thank you

WebReflection added a commit that referenced this issue Oct 15, 2024
Fix #3 - Avoid collisions
@WebReflection
Copy link
Member

WebReflection commented Oct 15, 2024

Aha, I found a (hopefully) smart way to have the source for free without a sweat while preserving, or preventing, collisions. It was a lovely call and gotcha to fix but if you have anything else I didn't think about please do let me know and I'll fix all the things.

The only "caveat" I see with this approach is malformed JSON that would throw anyway on parse or the possibility that a JSON contains 2**32-1 strings (as values, keys are not pushed) but I think at that point anyone using this module would have a "slightly bigger problem" and I don't need to cover terabytes of JSON parsing in my projects that need this feature. Beside that, I think this should work fairly well, fast, and compact 👋

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

No branches or pull requests

2 participants