feat(ngModel): add ngModelContext for getter/setter bindings #9865
Conversation
|
I'm sorry, but I wasn't able to verify your Contributor License Agreement (CLA) signature. CLA signature is required for any code contributions to AngularJS. Please sign our CLA and ensure that the CLA signature email address and the email address in this PR's commits match. If you signed the CLA as a corporation, please let us know the company's name. Thanks a bunch! PS: If you signed the CLA in the past then most likely the email addresses don't match. Please sign the CLA again or update the email address in the commit of this PR. |
|
Respectfully speaking, @mary-poppins, I think you're mistaken 😄 |
|
Why add a new directive, since we already have |
|
Yeah, that would make this a bit nicer, I could move this into a property Fundamentally the feature would be the same though, and it's still a little I'll update the PR to use ngModelOptions instead later today. On Nov 1, 2014 2:57 AM, "Georgios Kalpakas" notifications@github.com
|
|
Why add anything new? Could this not be done without adding to the API. Something such as... If using a hacky regex doesn't cover some cases then maybe |
|
I don't particularly like this kind of "opt-in" behaviour and would prefer to see If we did do this, there'd be no way to use the current behaviour, and all uses of |
|
Things like I think the default context should be null, not the window. You're right that could be a breaking change, but I'd hope not many people would be depending on that :| |
|
I thought of that case, but I don't think it's really an issue: isn't that an invalid getterSetter function? ngModel already assumes it can invoke it with an argument, i.e. |
|
Also if we are breaking the default context to not be window, I'd argue that the |
|
Ya maybe I think that case is valid - the ngModel expression gets executed to return a function, then that returned function is the |
|
Hrm, that's fair, but I don't think ngModel currently supports providing an expression that returns a getterSetter. You can see that here: http://plnkr.co/edit/pqyebcDFw4BjSpe8dJqT?p=preview |
|
Wait - nevermind, it does work like you described, I had a typo in there... Gross |
|
Also, I'm pretty sure your regex is defeated by any string with non-word characters, e.g. <input ng-model="contextObj.weirdLookupFn('the keyword is... !@#%^%')" ng-model-options="{ getterSetter: true }">My gut says that regexes can't solve the problem of determining the calling context generically, you need the tokenizing that |
|
That should still be covered by the second regex which ensures it ends with |
|
Hmm, maybe I'm misunderstanding you...? "contextObj.weirdLookupFn('the keyword is... !@#%^%')".match(/^(.+)\.[\w$]+$/)
// returns null |
|
That looks correct, it should be null and use the default context... |
|
OK I was misunderstanding you, I thought you were trying to get expressions like that to match and return It occurs to me that we could also just ask |
NevilleS
force-pushed
the
add-ng-model-context
branch
from
776472b to
591b036
Compare
|
OK, I updated this to automatically determine the context instead. I definitely prefer this... Let me know if it's still too weird to use, though, or if there's a case where |
|
That looks right. That test makes me lean toward making the default context null instead of the scope though, but that's just my opinion (and my examples below all use the scope as context still). Might be able to make it a bit simpler though? jbedard@6188949 Or honestly I think the current getterSetter implementation is weird how it allows plain values even if getterSetter is true, and I'm not sure if it intentionally allows ngModelOptions.getterSetter to change at any time (there's no tests enforcing that one). Removing those 2 abilities would be a breaking change but I think it would simplify things a lot and improve $watch performance a bit (in all cases, getterSetter or not): jbedard@3392351 Those are just ideas though. Someone on the angular team should probably make these decisions :P |
|
I like setting up the parsedNgModel context during I assume the Angular team will chime in eventually 😄 |
NevilleS
force-pushed
the
add-ng-model-context
branch
from
591b036 to
807d947
Compare
|
UPDATE: |
|
Yup, you're totally right... And since array notation uses an enclosed string which can have any characters, I think it safely rules out using a regex to determine the context, unless someone more well versed in regex-fu can write something that returns the correct context for all of these: As I see it there are four options:
Am I missing anything? What do you guys think makes the most sense? |
|
I don't really use
|
|
And that is what I was afraid would happen using a regex like that... I'd vote for jbedard@3392351 if the breaking change is acceptable. This will also improve performance a bit for the non getterSetter case. |
|
@gkalpak re: the "do nothing" case, as I understand it the folks who use Typescript can't just "use a closure" since it's generated code, or something like that. So not providing an option like an implicit or explicit context parameter leaves them out of the party 😢 |
|
😢 |
|
So if we've abandoned hope of an implicit way to determine the context, I'll just go back to the explicit version, provided via I still think that the default context should be changed from |
NevilleS
force-pushed
the
add-ng-model-context
branch
from
807d947 to
a9fa4d3
Compare
|
OK, updated. Still not using the performance improvements recommended by @jbedard since that's a larger change to the behaviour of |
…getter/setter bindings
Along with getterSetter, allow users to provide an expression via the getterSetterContext option.
This expression is evaluated to determine the context that should be used when invoking the ngModel
as a getter/setter function.
For example, <input ng-model="someObject.value" ng-model-options="{ getterSetter: true }"> would
previously invoke 'someObject.value()' from the global context. Now, users can specify context, like
ng-model-options="{ getterSetter: true, getterSetterContext: 'someObject'}", which would invoke
'someObject.value()' using 'someObject' as the calling context.
If getterSetterContext is not provided, fallback to using the current scope as the context.
Closes angular#9394
BREAKING CHANGE: previously, getter/setter functions would always be called from the global context.
This behaviour was unexpected by some users, as described in angular#9394, and is not particularly nice
anyways. Applications that relied on this behaviour can use `$window` instead of `this` to access
the global object... but they probably shouldn't be storing global state anyways!
NevilleS
force-pushed
the
add-ng-model-context
branch
from
a9fa4d3 to
c0747dd
Compare
|
I'd still hope we can do this without adding to the API, such as jbedard@3392351. But I guess that's not our decision... |
|
@jbedard hmm, not necessarily. That approach is programmatically modifying the ngModel expression to transform it into get/set method invocations. I'd have to think a bit more to see if that would work in all cases, since it makes some assumptions about the ngModel expression (namely that |
|
Ya I'd want to think about that a bit more too. But since Either way I like the idea of not changing the API. Even doing the regex thing for now - it only improves functionality and does not break any existing use cases (just has known bugs where it doesn't work). Then we can fix it or add more functionality later without having a public API to deal with. |
|
@btford I'm just assigning you because getters/setters was yours so you might have an opinion about whether we want this or not. If you don't want it, just pass it back I guess |
|
I'll take care of this this week. On Tue Nov 11 2014 at 11:25:16 AM Caitlin Potter notifications@github.com
|
|
I haven't come up with any instances where |
|
Hey @btford: this isn't quite ready to merge yet (I should update the docs for usage first), but let me know if the proposed API here makes sense. |
|
Here's my take –> #10136. |
Many thanks to @NevilleS and @jbedard for collaborating with me on a solution to this! Closes angular#9394 Closes angular#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`.
Many thanks to @NevilleS and @jbedard for collaborating with me on a solution to this! Closes angular#9394 Closes angular#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`.
Many thanks to @NevilleS and @jbedard for collaborating with me on a solution to this! Closes angular#9394 Closes angular#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`.
Many thanks to @NevilleS and @jbedard for collaborating with me on a solution to this! Closes angular#9394 Closes angular#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`.
Many thanks to @NevilleS and @jbedard for collaborating with me on a solution to this! Closes angular#9394 Closes angular#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`.
Adds an optional attribute,
ng-model-context, that will be evaluated to provide the callingcontext used when invoking the ngModel getter/setter function. When not provided, falls back to the
existing behavior of invoking getter/setter functions from the global context.
Closes #9394