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

fix(ngModelOptions): preserve context of getter/setters #10136

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@btford
Contributor

btford commented Nov 20, 2014

Closes #9394

BREAKING CHANGE: previously, ngModel invoked getter/setters in the global context.

For example:

<input ng-model="model.value" ng-model-options="{ getterSetter: true }">

would previously invoke model.value() in the global context.

Now, ngModel invokes value with model as the context.

It's unlikely that real apps relied on this behavior. If they did they can use .bind to explicilty
bind a getter/getter to the global context, or just reference globals normally without this.

@googlebot

This comment has been minimized.

Show comment
Hide comment
@googlebot

googlebot Nov 20, 2014

Thanks for your pull request.

It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://cla.developers.google.com/.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check the information on your CLA or see this help article on setting the email on your git commits.

Once you've done that, please reply here to let us know. If you signed the CLA as a corporation, please let us know the company's name.

googlebot commented Nov 20, 2014

Thanks for your pull request.

It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://cla.developers.google.com/.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check the information on your CLA or see this help article on setting the email on your git commits.

Once you've done that, please reply here to let us know. If you signed the CLA as a corporation, please let us know the company's name.

@googlebot googlebot added the cla: no label Nov 20, 2014

@btford

This comment has been minimized.

Show comment
Hide comment
@btford

btford Nov 20, 2014

Contributor

oh googlebot, you so crazy

Contributor

btford commented Nov 20, 2014

oh googlebot, you so crazy

@googlebot

This comment has been minimized.

Show comment
Hide comment
@googlebot

googlebot Nov 20, 2014

CLAs look good, thanks Bri Bri!

googlebot commented Nov 20, 2014

CLAs look good, thanks Bri Bri!

@jbedard

This comment has been minimized.

Show comment
Hide comment
@jbedard

jbedard Nov 20, 2014

Contributor

Another question... do we intentionally support the ngModelOptions object being modified after $$setOptions is called?

<input ng-model="model.value" ng-model-options="{ getterSetter: variableThatChangesRandomly }">

My comment about changing ngModelGet = parsedNgModel and overriding at $$setOptions would remove support for this because we would read getterSetter only once. I think removing support for this is good and removes a bunch of weird edge cases, but it could be considered a breaking change...

Contributor

jbedard commented Nov 20, 2014

Another question... do we intentionally support the ngModelOptions object being modified after $$setOptions is called?

<input ng-model="model.value" ng-model-options="{ getterSetter: variableThatChangesRandomly }">

My comment about changing ngModelGet = parsedNgModel and overriding at $$setOptions would remove support for this because we would read getterSetter only once. I think removing support for this is good and removes a bunch of weird edge cases, but it could be considered a breaking change...

@btford

This comment has been minimized.

Show comment
Hide comment
@btford

btford Nov 20, 2014

Contributor

do we intentionally support the ngModelOptions object being modified

Nope. I agree about this being a potential source of headaches. I think getter/setters already gives devs enough dynamicity.

Contributor

btford commented Nov 20, 2014

do we intentionally support the ngModelOptions object being modified

Nope. I agree about this being a potential source of headaches. I think getter/setters already gives devs enough dynamicity.

@jbedard

This comment has been minimized.

Show comment
Hide comment
@jbedard

jbedard Nov 20, 2014

Contributor

I think something a little closer to jbedard@3392351 would be nice then...

Contributor

jbedard commented Nov 20, 2014

I think something a little closer to jbedard@3392351 would be nice then...

@btford

This comment has been minimized.

Show comment
Hide comment
@btford

btford Nov 20, 2014

Contributor

@jbedard – nice!

$parse already memoizes parsed expressions, so I don't think your change adds too much in terms of run-time performance (just a few lookups, maybe a bit of GC pressure from string concats), and possibly introduces additional memory costs from the additional closure.

I think if we continue discussion down this path we should have a benchmark for it.

Contributor

btford commented Nov 20, 2014

@jbedard – nice!

$parse already memoizes parsed expressions, so I don't think your change adds too much in terms of run-time performance (just a few lookups, maybe a bit of GC pressure from string concats), and possibly introduces additional memory costs from the additional closure.

I think if we continue discussion down this path we should have a benchmark for it.

@NevilleS

This comment has been minimized.

Show comment
Hide comment
@NevilleS

NevilleS Nov 20, 2014

Contributor

The "trick" of adding () is nice because it doesn't require an API change and kinda just does the "right" thing. It breaks in at least one odd case, for expressions like: ng-model="someService.expr;", since appending parentheses after a semicolon explodes as you might expect.

But if you can make the error message helpful in that case, maybe it's OK?

Contributor

NevilleS commented Nov 20, 2014

The "trick" of adding () is nice because it doesn't require an API change and kinda just does the "right" thing. It breaks in at least one odd case, for expressions like: ng-model="someService.expr;", since appending parentheses after a semicolon explodes as you might expect.

But if you can make the error message helpful in that case, maybe it's OK?

@NevilleS

This comment has been minimized.

Show comment
Hide comment
@NevilleS

NevilleS Nov 20, 2014

Contributor

Amusingly, something else like ng-model="someService.getterSetter || fallbackService.getterSetter" would have some interesting behaviour when you append parentheses too 😄

Contributor

NevilleS commented Nov 20, 2014

Amusingly, something else like ng-model="someService.getterSetter || fallbackService.getterSetter" would have some interesting behaviour when you append parentheses too 😄

@jbedard

This comment has been minimized.

Show comment
Hide comment
@jbedard

jbedard Nov 20, 2014

Contributor

:|

We could do $parse("(" + ngModel + ")()"), I think losing the context in that case would be correct...

Contributor

jbedard commented Nov 20, 2014

:|

We could do $parse("(" + ngModel + ")()"), I think losing the context in that case would be correct...

@jbedard

This comment has been minimized.

Show comment
Hide comment
@jbedard

jbedard Nov 20, 2014

Contributor

Actually what I said in that last comment won't work. Doing (ngModel)() will make it lose the context in every case :(

I've updated mine to be not as breaking jbedard@081195a. The main advantage is that it only checks options.getterSetter once on initialization and if it is false the watcher method invoked per-digest is much simpler (better performance). I'm thinking this change is breaking enough we may as well break one more thing and fulfill the todo I added in that commit though. That would make it a bit simpler and better performance for the getterSetter case as well.

Sorry if I'm spamming this PR. Should I open another one instead?

Contributor

jbedard commented Nov 20, 2014

Actually what I said in that last comment won't work. Doing (ngModel)() will make it lose the context in every case :(

I've updated mine to be not as breaking jbedard@081195a. The main advantage is that it only checks options.getterSetter once on initialization and if it is false the watcher method invoked per-digest is much simpler (better performance). I'm thinking this change is breaking enough we may as well break one more thing and fulfill the todo I added in that commit though. That would make it a bit simpler and better performance for the getterSetter case as well.

Sorry if I'm spamming this PR. Should I open another one instead?

@NevilleS

This comment has been minimized.

Show comment
Hide comment
@NevilleS

NevilleS Nov 20, 2014

Contributor

I don't think your updated PR handles the weird cases I was talking about... the test passes, but only because your ternary expression is wrapped in brackets 😃

In any case, I think the examples I provided were pretty contrived. 99.999% of ngModel expressions are just simple scope bindings like data or user.name, so implicitly appending () to them will turn those into getters, and solve the initial problem in #9394, which was that services/typescript objects were being invoked without the necessary context. Therefore, I think your earlier one (jbedard/angular.js@3392351) is pretty close, I'd just think to cover the bases we should improve the error message to provide some guidance in the weird cases where $attrs.ngModel + "()" results in an invalid expression, because currently I'd be pretty confused to see a $parse error for an expression that I didn't even write!

Unless of course someone can come up with a non-contrived counter example where this won't work, because I still have an uneasy feeling I can't quite shake...

Contributor

NevilleS commented Nov 20, 2014

I don't think your updated PR handles the weird cases I was talking about... the test passes, but only because your ternary expression is wrapped in brackets 😃

In any case, I think the examples I provided were pretty contrived. 99.999% of ngModel expressions are just simple scope bindings like data or user.name, so implicitly appending () to them will turn those into getters, and solve the initial problem in #9394, which was that services/typescript objects were being invoked without the necessary context. Therefore, I think your earlier one (jbedard/angular.js@3392351) is pretty close, I'd just think to cover the bases we should improve the error message to provide some guidance in the weird cases where $attrs.ngModel + "()" results in an invalid expression, because currently I'd be pretty confused to see a $parse error for an expression that I didn't even write!

Unless of course someone can come up with a non-contrived counter example where this won't work, because I still have an uneasy feeling I can't quite shake...

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Nov 21, 2014

Contributor

(tentatively moving this to the next milestone --- rearrange at your discretion)

Contributor

caitp commented Nov 21, 2014

(tentatively moving this to the next milestone --- rearrange at your discretion)

@btford

This comment has been minimized.

Show comment
Hide comment
@btford

btford Nov 22, 2014

Contributor

I like the approach in jbedard@081195a.

I've updated my PR accordingly, and made the tests around this feature more explicit.

I'm not interested in supporting weird ternary expressions for ngModelOptions. If you want conditional logic like that, it should belong in a function within your scope. There's no reason to allow yet another level of indirection in the template there.

Contributor

btford commented Nov 22, 2014

I like the approach in jbedard@081195a.

I've updated my PR accordingly, and made the tests around this feature more explicit.

I'm not interested in supporting weird ternary expressions for ngModelOptions. If you want conditional logic like that, it should belong in a function within your scope. There's no reason to allow yet another level of indirection in the template there.

@jbedard

This comment has been minimized.

Show comment
Hide comment
@jbedard

jbedard Nov 22, 2014

Contributor

That approach will also help with #9609 by removing the wrapper and options/getterSetter check from the watch function.

If you want to simplify it one step further, with an additional breaking change, then jbedard@3392351 is even simpler where getterSetter mode only supports getterSetter functions and no longer supports plain values. I think this makes things simpler, but it is a bigger breaking change.

Contributor

jbedard commented Nov 22, 2014

That approach will also help with #9609 by removing the wrapper and options/getterSetter check from the watch function.

If you want to simplify it one step further, with an additional breaking change, then jbedard@3392351 is even simpler where getterSetter mode only supports getterSetter functions and no longer supports plain values. I think this makes things simpler, but it is a bigger breaking change.

@lgalfaso

This comment has been minimized.

Show comment
Hide comment
@lgalfaso

lgalfaso Nov 22, 2014

Member

otherwise, LGTM

Member

lgalfaso commented Nov 22, 2014

otherwise, LGTM

fix(ngModelOptions): preserve context of getter/setters
Many thanks to @NevilleS and @jbedard for collaborating with me on a solution to this!

Closes #9394
Closes #9865

BREAKING CHANGE: previously, ngModel invoked getter/setters in the global context.

For example:

```js
<input ng-model="model.value" ng-model-options="{ getterSetter: true }">
```

would previously invoke `model.value()` in the global context.

Now, ngModel invokes `value` with `model` as the context.

It's unlikely that real apps relied on this behavior. If they did they can use `.bind` to explicilty
bind a getter/getter to the global context, or just reference globals normally without `this`.
@btford

This comment has been minimized.

Show comment
Hide comment
@btford

btford Nov 22, 2014

Contributor

Landed as bb4d3b7.

Thanks everyone! 👯

Contributor

btford commented Nov 22, 2014

Landed as bb4d3b7.

Thanks everyone! 👯

@btford btford closed this Nov 22, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment