PolymerElements / app-localize-behavior

Polymer behaviour to help internationalize your application
48 stars 54 forks source link

Add missing .bind(this) #108

Closed JosefJezek closed 6 years ago

notwaldorf commented 7 years ago

Not sure I understand why this is needed __computeLocalize is a function on the Polymer element, so the this inside of it is correct, and it's the element itself (it's not fired from an event or anything

JosefJezek commented 6 years ago

@notwaldorf this is needed for this.useKeyIfMissing

googlebot commented 6 years ago

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


JosefJezek commented 6 years ago

I signed it!

googlebot commented 6 years ago

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

JosefJezek commented 6 years ago

I signed it!

JosefJezek commented 6 years ago

I signed it!

JosefJezek commented 6 years ago

@notwaldorf this.useKeyIfMissing is in anonymous function...

https://github.com/StartPolymer/app-localize-behavior/blob/master/app-localize-behavior.html#L230

JosefJezek commented 6 years ago

I signed it!

googlebot commented 6 years ago

CLAs look good, thanks!

sks commented 6 years ago

@JosefJezek : Any words on the status of this PR ?

sks commented 6 years ago

@notwaldorf : Anything blocking from merging this PR ?

davidrleonard commented 6 years ago

@notwaldorf Wanted to chime in and thank you for taking the time to help us out with this issue. This fix will solve some problems for us. It seems to be linked to some deeper problems in how Polymer's template decoding code treats methods/arguments used in the template.

Here's a reproduction of the issue: https://codepen.io/davidleonard/pen/GQqGgE

When a method is defined in a Polymer element's template that takes localize as one of its arguments (e.g. <button>[[_getLabel(showFallbackLabel, localize)]]</button>), the localize function's this scope is redefined to the window. This breaks things, because the useKeyIfMissing property is defined on the element instance, not on the window. So the fallback behavior breaks here:

https://github.com/PolymerElements/app-localize-behavior/blob/6f36f0d6a57df7713df99a1aa2f350a32ef4995c/app-localize-behavior.html#L260-L271

When this.useKeyIfMissing is read, the this scope is window, so its looking for window.useKeyIfMissing. :(

This seems to be some weird but that only shows up when the localize method is used as an argument for a method in a Polymer template, not when it is called directly (e.g. <span>[[localize('FallbackLabel')]]</span>). But accepting this PR and calling .bind(this) on the anonymous function seems to solve this issue.

Thanks again, cheers!

notwaldorf commented 6 years ago

Eeeek sorry i dropped the ball on this folks. Looks good!

notwaldorf commented 6 years ago

(let me cycle the builds as a sanity check)