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

Projects
None yet
4 participants
@pimterry
Copy link
Contributor

pimterry commented Mar 29, 2019

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.

pimterry added some commits Mar 29, 2019

Add support for serializing ES6 sets & maps
Note that this requires Array.from, or an Array.from
polyfill (necessary in IE11 or Node < 4)
@yahoocla

This comment has been minimized.

Copy link

yahoocla commented Mar 29, 2019

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 has been minimized.

Copy link

krutzler commented Mar 29, 2019

@pimterry

This comment has been minimized.

Copy link
Contributor Author

pimterry commented Mar 29, 2019

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

@pimterry pimterry referenced this pull request Mar 29, 2019

Closed

Support for Set/Map #12

@okuryu

This comment has been minimized.

Copy link
Collaborator

okuryu commented Mar 31, 2019

Thanks! It looks good to me.

@pimterry

This comment has been minimized.

Copy link
Contributor Author

pimterry commented Apr 10, 2019

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

This comment has been minimized.

Copy link
Collaborator

okuryu commented Apr 11, 2019

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

@okuryu

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor Author

pimterry commented Apr 14, 2019

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

This comment has been minimized.

Copy link
Collaborator

okuryu commented Apr 15, 2019

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

@pimterry

This comment has been minimized.

Copy link
Contributor Author

pimterry commented Apr 15, 2019

@okuryu sure, done 👍

@okuryu

okuryu approved these changes Apr 16, 2019

Copy link
Collaborator

okuryu left a comment

Thanks! 👍

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

2 checks passed

cla/licenses User has a valid Oath CLA
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@okuryu

This comment has been minimized.

Copy link
Collaborator

okuryu commented Apr 16, 2019

Published serialize-javascript@1.7.0. Thanks!

@pimterry pimterry deleted the pimterry:sets-and-maps branch Apr 16, 2019

@pimterry

This comment has been minimized.

Copy link
Contributor Author

pimterry commented Apr 16, 2019

Amazing, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.