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.

Sign up for GitHub

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

Jump to bottom

Binding context exports unnecessary properties which could shadow globals. #2294

Closed
bennieswart opened this issue Sep 22, 2017 · 3 comments · Fixed by #2296
Closed

Binding context exports unnecessary properties which could shadow globals. #2294

bennieswart opened this issue Sep 22, 2017 · 3 comments · Fixed by #2296
Labels
affected: average severity: minor type: bug
Milestone
3.5.0

Comments

@bennieswart
Copy link
Contributor

In the file bindingAttributeSyntax.js a _subscribable property is created on a binding context under certain conditions. During build, this property is minified and in the 3.4.2 release it is exported as Q. See this jsfiddle for an example: https://jsfiddle.net/s0wf1hga/1/. Note the errors in console.

This becomes a problem when a global property with the same name exists and referenced from inside bindings. Granted, the chance of a name conflict is rare. I did, however, encounter a problem with a custom build I was making where _subscribable was minified to $, which shadowed jquery in all bindings and broke a lot of things.

The workarounds for this problem are many, I know, but rather than a workaround I feel it is more right and rather easy to remove the problem. I'm also not saying that all unnecessary properties everywhere should be hidden, but the binding context is a rather unique situation with more ramifications than usual.
@mbest
Copy link
Member

mbest commented Sep 22, 2017

Those are good points. Perhaps we shouldn't minify _subscribable.

@mbest mbest added this to the 3.5.0 milestone Sep 22, 2017
@bennieswart
Copy link
Contributor Author

Not minifying _subscribable is the workaround I ended up using.
Ideally I suppose one would want to 'hide' the property on the instance, for example by using Symbols. Is there a way to do this across all the currently supported browser versions?
If not I'd be happy to submit a PR which prevents _subscribable from being minified, as well as a test case if you think it useful.

@mbest
Copy link
Member

mbest commented Sep 22, 2017

We could use a Symbol in browsers that support it and a non-minified property on the others. We have that already for ko.computed: https://github.com/knockout/knockout/blob/master/src/subscribables/dependentObservable.js#L1

bennieswart added a commit to bennieswart/knockout that referenced this issue Sep 23, 2017
@bennieswart
Hide or don't minify extra internal properties on binding context
f5baa12 
It is possible for properties such as `_subscribable` to be created on a
binding context, which, after minification, could be renamed to any
arbitrary character. In most cases this is not an issue, except when a
global variable with the same name exists, however rare the case may be.
This causes the global variable to be shadowed in all bindings, which is
especially problematic in the case of, say, `$`, which breaks jquery.

To fix this, either hide such properties with a Symbol, or simply don't
minify them so that the name is constant and predictable.

Fixes knockout#2294
@bennieswart bennieswart mentioned this issue Sep 23, 2017
Merged
bennieswart added a commit to bennieswart/knockout that referenced this issue Jan 25, 2019
@bennieswart
Hide or don't minify extra internal properties on binding context
0062952 
It is possible for properties such as `_subscribable` to be created on a
binding context, which, after minification, could be renamed to any
arbitrary character. In most cases this is not an issue, except when a
global variable with the same name exists, however rare the case may be.
This causes the global variable to be shadowed in all bindings, which is
especially problematic in the case of, say, `$`, which breaks jquery.

To fix this, either hide such properties with a Symbol, or simply don't
minify them so that the name is constant and predictable.

Fixes knockout#2294
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
affected: average severity: minor type: bug
Projects
None yet
Milestone
3.5.0
Development

Successfully merging a pull request may close this issue.

Hide or don't minify extra internal properties on binding context bennieswart/knockout
2 participants
@mbest @bennieswart