-
Notifications
You must be signed in to change notification settings - Fork 181
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
Conversation
Note that this requires Array.from, or an Array.from polyfill (necessary in IE11 or Node < 4)
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! 😄 |
This comment was marked as outdated.
This comment was marked as outdated.
(Doesn't seem to have updated, but I have now signed the CLA) |
Thanks! It looks good to me. |
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. |
Sorry for delay! I'll work on this in this week! |
I have only one concern. Regarding |
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. Option 3, we could just document it more explicitly. How would you feel if I just added the below to the docs?
|
Okay, let's take your option 3. Would you mind tweaking the README? |
@okuryu sure, done 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! 👍
Published |
Amazing, thank you! |
This fixes #12
It's fairly simple: the map/set contents are serialized as arrays (using
map.entries()
orset.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 toserialize
(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.