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

Replace objects with Maps to ensure compatibility with Hermes #215

Open
wants to merge 1 commit into
base: legacy
Choose a base branch
from

Conversation

danii1
Copy link

@danii1 danii1 commented Feb 21, 2024

This PR replaces objects with Map to alleviate the issue with Hermes runtime(React Native) that doesn't support a large number of props in an object. This allows to use this package in React Native apps.

Related issue:
#168

@Rycochet
Copy link
Collaborator

This needs to compare with #98 and have a look at any side effects etc mentioned - not got time to review myself now, but of the general opinion that it's worth adding :-)

@danii1
Copy link
Author

danii1 commented Feb 21, 2024

I have another branch with the same changes applied to v2: https://github.com/DISCO-team/lz-string/tree/feature/object-map

Ran test:bench, Map seems faster in most scenarios(particularly where it matters like compressing large text), not surprising given it's optimized for addition and deletion:
obj.txt
map.txt

Changes in this PR are straightforward and simpler than in #98

@adbs1
Copy link

adbs1 commented May 9, 2024

@Rycochet @danii1 Any further progress likely here? The changes look trivial with a cursory glance :)

@karnthis
Copy link
Contributor

The change is solid and has my vote of support from a code standpoint, but needs a final decision from a compatibility angle. LZ-String has been maintaining compatibility with anything javascript released since roughly 2012, and this change will break that trend. I believe there is a limit to backwards compatibility and a move to v2 is a good time to revisit that support, but is ultimately not my decision to make. @Rycochet @pieroxy what are your thoughts?

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.

4 participants