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

Deserialize PHP associative arrays as new Map() #295

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

Conversation

miso-belica
Copy link
Contributor

This way the order of the original PHP array is always preserved. It took me a while, sorry 😅

closes #293

This way the order of the original PHP array is always preserved.
@miso-belica
Copy link
Contributor Author

Hello @steelbrain I know it's not easy to find a time for PRs sometimes but this one is not so big so if you find some it would be great 🙂

@steelbrain
Copy link
Owner

Hi @miso-belica! Sorry it took so long. I'll review this as soon as I can and get back to you. Appreciate the contribution

@steelbrain steelbrain self-requested a review February 25, 2024 16:00
@steelbrain
Copy link
Owner

@miso-belica There's some changes we're going to have to make to this before we can ship it, lmk if you want to make them on your own. I'd also be down to making them on my end. Anyway, here's the review requests:

  • Git seems to recognize the test files as binary, so add a .gitattributes file where we're forcing it to be text
  • Since it's a behavior-breaking change and would require semver-major update, let's make preserveOrder false by default, and opt-in. Maybe it can be true by default in the next semver-major release
  • [].reduce(...) with objects being merged has been described as inefficient, I'd like us to move away from it. See https://prateeksurana.me/blog/why-using-object-spread-with-reduce-bad-idea/
  • Instead of modifying existing tests, I'd like us to keep them in place and just add new tests instead

@steelbrain steelbrain removed their request for review February 28, 2024 00:07
@miso-belica
Copy link
Contributor Author

@steelbrain thank you for the article and other hints. Recently I was blessed and was able to remove WP from the project so I hope I will remove this library soon :) But, thank you for it. It helped us to maintain the project for years. Feel free to make any modification you find useful to merge the code. I hope it helps prevents some bugs to people stuck with WP in their project.

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.

Deserialize PHP associative arrays as new Map()
2 participants