-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Enable ko.utils.extend to also extend Symbols #2436
base: master
Are you sure you want to change the base?
Conversation
@mbest would you mind giving some feedback here? Do you think it is a valid use case? |
I do see the value of copying symbol properties, but I'm concerned that this would be a breaking change. |
@mbest what would be the protocol for merging this if it is a breaking change, albeit an improvement? In any case, I suspect the number of users that could possibly experience a break due to this would be minuscule. PS Couldn't we get away with some low-probability breaking changes in 3.5? |
If all the existing tests work, isn't that enough to assert that it doesn't lead to a breaking change? |
This should not be the default behavior. All extending/merging/assigning methods of any library I know (underscore, jquery, lodash) or native code ignore the non-enumerable properties. So while I have actually needed to extend something with symbols before, I still think you should hide this feature behind a flag or use a different method name. It is just not the expected behavior of such a method. |
I think @uncaught raises a valid point. The default behaviour for |
I know, old habbits are hard to die in code when the standards use other namings like Object.assign(). Object.getOwnPropertyNames doesn't list Symbol So what about 'extendWithSymbols' or 'assignWithSymbols'? |
We often use symbols in binding contexts to store intermediate data used by bindings, and when creating a child binding context, these symbols should be copied along with the normal properties. This is done using
ko.utils.extend
underlying.