Support for symbols as keys

homer0 commented 5 years ago

When you merge objects that use symbols as keys, they get removed.

I did some research and testing and I believe the issue is here:

https://github.com/justmoon/node-extend/blob/ef6192a02057fce77ec8d75e37aa3760fd509e87/index.js#L88

The for ... in ignores the symbols, so I tried changing it a little bit:

- for (name in options) {
+ const names = [
+     ...Object.getOwnPropertySymbols(options),
+     ...Object.keys(options)
+ ];
+ for (name of names) {

And now it works.

The spread can be replaced with concat, but getOwnPropertySymbols pretty much leaves IE out and I don't know which are the browser/engines the library should support, that's why I didn't send a PR.

Let me know what you think. Thanks.

ljharb commented 5 years ago

This package is a port of jquery's extend method. Does that support symbols?

homer0 commented 5 years ago

@ljharb we both know that it doesn't.

I never imagined this project was limited to what jQuery does or doesn't support. More so seeing that #35 was considered at one point.

Can you add something on the README or a CONTRIBUTING file?

Sorry for waisting your time!

ljharb commented 5 years ago

The readme already says it’s a port of jquery’s extend.

homer0 commented 5 years ago

@ljharb yes, but that doesn't mean that you won't make improvements or accept suggestions; those are two different things.

ljharb commented 5 years ago

That’s true, but if you want to include symbols, you can use object spread or Object.assign already?

homer0 commented 5 years ago

@ljharb yep, but not for deep merge :P. No worries, I'll code something, is not that complicated :D.

justmoon / node-extend

Support for symbols as keys #52