fix(input): prevent double $digest when using jQuery trigger. #5293
Conversation
|
Thanks for the PR!
If you need to make changes to your pull request, you can update the commit with Thanks again for your help! |
| @@ -515,6 +515,24 @@ describe('input', function() { | |||
| 'event so that form auto complete works',function() { | |||
| assertBrowserSupportsChangeEvent(true); | |||
| }); | |||
|
|
|||
| if (jqLite().jquery) { | |||
There was a problem hiding this comment.
Is this how you detect jQuery in tests? Should I do it differently?
There was a problem hiding this comment.
There seems to be a variety of ways!!
if(!_jqLiteMode)
if (!jqLite.fn)
if (window.jQuery)
I guess the first is the right method for internal tests
|
This seems like a more general problem with JQuery.trigger, which should be dealt with outside of input... |
|
@petebacondarwin Hmm, maybe it would be better to monkey-patch The test should be fine, though. :) |
There has got to be a better way |
maybe it would be better to monkey-patch Than monkey-patching? I don't think so. Handlers can't be modified for various reasons and you have to handle both situations with native event dispatch and ones with jQuery.fn.trigger.
|
|
The problem is that it becomes really, really hard to test this, as people are using different versions of jQuery, and by monkey-patching you end up having to essentially support all of them seperately, it seems like an absolute nightmare. If there is any other solution at all, it would be the one to use :| |
|
@caitp I agree it would be better not to monkey-patch, though Angular already monkey-patches a couple of key jQuery methods. On a further thought, this change could be more disrupting as changing synchronous function to an asymchronous one is a serious thing and such a change could break a LOT of code. The only other solution I can think of is to go through the Angular code base and add such guards as I added in this PR to every single event handler that changes sth in scope. Such a policy would have to be strictly followed in the future. For handlers defined in user code the user would have to care to make similar amendments to all handlers influencing scope. I prefer the second approach, I agree making trigger async can be an extremely dangerous thing to do. Thoughts? |
|
(the policy I described would go away as soon as Angular drops $digest but not before that happens) |
|
Another option would be to monkey-patch 'on' so that it wraps all handlers in a conditional scope.$apply depending on event.isTrigget value. This would break almost every current code (it would require people to remove their scope.$apply wrapper around handlers bodies), though so that wouldn't be an option for now and it still would be dangerous. I don't see a better option than to extend what current PR does to other places in the code base. |
|
no, monkey-patching 'on' or 'bind' or anything else like that is not appropriate behaviour either... I'm not sure what the right thing to do here is, but I don't think monkey-patching jQuery is the right thing to do, just because there's no guarantee that we won't break peoples apps who happen to be using some version of JQ which has slightly different behaviour. I don't know specifically how stable that particular property's behaviour is, but we can't really test for every canonical version of jQuery ever released, and it's not good to say "we only support 2.0.3" or something (because people are definitely using older versions for IE7 compatibility, among other reasons) These are just my personal thoughts -- One of the core team might have a different opinion on the matter |
|
@caitp You didn't comment on my preferred solution that involves no additional monkey-patching. :) |
|
I think my comment was that "I'm not sure what to do about it"... It would be good to come up with something, because clearly it's a problem (although not one I've ever seen with 1.2.3 + jquery 2.0.3) But monkey-patching external code of which many versions are in use, seems like a terrible idea to me. |
ghost
assigned 
If an event was performed natively, jQuery sets the isTrigger property. When triggering event manually, the field is not present. Manually triggered events are performed synchronously which causes the "$digest already in progress" error.
|
+1 |
1 similar comment
|
+1 |
|
Erm, I realize I said earlier today that it looked like my comments were addressed re: not monkey patching, there are some things that come up on closer inspection, which might be good to lookat as a followup patch if you're interested @mzgol. Primarily, the test... 2 things. One, it might be good to put together an e2e test so that we can make sure that this works correctly with native triggers. 2, the spec could be cleared up a lot. It appears that you're expecting the test not to throw, but this isn't explicitly done anywhere in the spec, and there seems to be a lot of unrelated stuff happening as well. That should probably be cleaned up. If you want to take care of that and submit a followup that would be awesome, or I can take a stab at it. I don't think we have a very good way to set up e2e tests apart from the docs e2e stuff right now, though. |
|
This merged commit us breaking apps at Google. We need to investigate this and we might need to revert this change. |
|
Yes, sorry for giving the impression that this was totally merge-able, I really only meant that my comments about monkey-patching had been addressed. It would be super-helpful to get some e2e tests into this patch so that we can be sure it doesn't break things. |
|
The issue is that the assumption that all manually triggered change events triggered from within an apply is invalid. It is possible to have a jquery plugin trigger a change event that ng-model listens on and in this case the view value change will go unobserved until the next digest. |
|
I fixed the issue by checking |
If an event was performed natively, jQuery sets the isTrigger property. When triggering event manually, the field is not present. Manually triggered events are performed synchronously which causes the "$digest already in progress" error. Closes angular#5293
If an event was performed natively, jQuery sets the isTrigger property. When triggering event manually, the field is not present. Manually triggered events are performed synchronously which causes the "$digest already in progress" error. Closes angular#5293
If an event was performed natively, jQuery sets the isTrigger property.
When triggering event manually, the field is not present. Manually
triggered events are performed synchronously which causes the "$digest
already in progress" error.