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

Enable ko.utils.extend to also extend Symbols #2436

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bennieswart
Copy link
Contributor

@bennieswart bennieswart commented Jan 25, 2019

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.

@bennieswart
Copy link
Contributor Author

@mbest would you mind giving some feedback here? Do you think it is a valid use case?

@mbest
Copy link
Member

mbest commented Feb 22, 2019

I do see the value of copying symbol properties, but I'm concerned that this would be a breaking change.

@bennieswart
Copy link
Contributor Author

@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?

@chrisknoll
Copy link

If all the existing tests work, isn't that enough to assert that it doesn't lead to a breaking change?

@uncaught
Copy link

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.

@bennieswart
Copy link
Contributor Author

I think @uncaught raises a valid point. The default behaviour for ko.utils.extend should perhaps be to exclude non-enumerable properties.
Still though, I think it's useful to have the option to include symbols when the binding context is extended. Perhaps we should hide this functionality in ko.utils.extend behind a flag and set that flag from the call to extend the binding context.

@the-djmaze
Copy link

the-djmaze commented Nov 13, 2020

I know, old habbits are hard to die in code when the standards use other namings like Object.assign().
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/assign

Object.getOwnPropertyNames doesn't list Symbol
Object.getOwnPropertySymbols does that

So what about 'extendWithSymbols' or 'assignWithSymbols'?

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.

None yet

5 participants