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

Add support for serializing ES6 sets & maps #45

Merged
merged 3 commits into from Apr 16, 2019

Conversation

pimterry
Copy link
Contributor

This fixes #12

It's fairly simple: the map/set contents are serialized as arrays (using map.entries() or set.values()), which can be passed straight back to their constructor to reconstitute them, and the map/set contents are recursively serialized in turn with an extra call to serialize (passing the same options).

Note that this requires Array.from. That means it doesn't work in IE, or in Node < 4. Map/Sets are only available in IE11+ though, and Node 0.12+, so there's a very small set of environments where you could actually hit that, and judging from your current test setup that's acceptable.

If Map/Set serialization is needed in those environments it just needs an Array.from polyfill.

Note that this requires Array.from, or an Array.from
polyfill (necessary in IE11 or Node < 4)
@yahoocla
Copy link

Thank you for submitting this pull request, however I do not see a valid CLA on file for you. Before we can merge this request please visit https://yahoocla.herokuapp.com/ and agree to the terms. Thanks! 😄

@krutzler

This comment was marked as outdated.

@pimterry
Copy link
Contributor Author

(Doesn't seem to have updated, but I have now signed the CLA)

@pimterry pimterry mentioned this pull request Mar 29, 2019
@okuryu
Copy link
Collaborator

okuryu commented Mar 31, 2019

Thanks! It looks good to me.

@pimterry
Copy link
Contributor Author

If you're happy with it then @okuryu, can we merge and release this? Anything else that needs doing first? Let me know if there's any way I can help.

@okuryu
Copy link
Collaborator

okuryu commented Apr 11, 2019

Sorry for delay! I'll work on this in this week!

@okuryu
Copy link
Collaborator

okuryu commented Apr 14, 2019

I have only one concern. Regarding Array.from isn't supported on IE11, I would like to avoid use it if possible. I'm worried that the same problem may recur as there was a report (#41) in the past for ES5 compatibility. If we accept this change, we might need to change the version to v2.0.0. Thought?

@pimterry
Copy link
Contributor Author

It's less risky than the arrow function issue, because this is just using a new function, not new syntax. You should never hit issues with generic minification or linting tools as in #41 - it's only at runtime, and only if you're using IE11 and you're serializing Map/Set objects and you're not using any modern polyfills. The support is much better than arrow functions too: arrow function support starts at Node 6, while this works for Node 0.12+.

If you do want to fix it, there's a few options. .entries() and .values() always return an iterator, so you need some way to convert that to an array. You could include a standard Array.from polyfill, but it's a little messy. Alternatively we could build our own simplified polyfill here, that only supports this one iterator case, but then you're maintaining your own unique polyfill.

Option 3, we could just document it more explicitly. How would you feel if I just added the below to the docs?

Please note that serialization for Sets & Maps requires support for Array.from (not available in IE or Node < 0.12), or an Array.from polyfill.

@okuryu
Copy link
Collaborator

okuryu commented Apr 15, 2019

Okay, let's take your option 3. Would you mind tweaking the README?

@pimterry
Copy link
Contributor Author

@okuryu sure, done 👍

Copy link
Collaborator

@okuryu okuryu left a comment

Choose a reason for hiding this comment

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

Thanks! 👍

@okuryu okuryu merged commit a5d6837 into yahoo:master Apr 16, 2019
@okuryu
Copy link
Collaborator

okuryu commented Apr 16, 2019

Published serialize-javascript@1.7.0. Thanks!

@pimterry pimterry deleted the sets-and-maps branch April 16, 2019 12:29
@pimterry
Copy link
Contributor Author

Amazing, thank you!

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.

Support for Set/Map
4 participants