fix($jsonpCallbacks): do not overwrite callbacks added by other apps #14824
Conversation
gkalpak
force-pushed
the
fix-jsonp-callbacks-with-multi-apps
branch
from
1cfcd2c to
5288866
Compare
| var callbackMap = {}; | ||
| var callbacks = $window.angular.callbacks = $window.angular.callbacks || {}; |
There was a problem hiding this comment.
Nope 😃
This is basically doing:
foo = foo || {}; // foo <--- $window.angular.callbacks
var bar = foo; // bar <--- callbacksThe idea is that if $window.angular.callbacks has already been initialized (e.g. by another app running on the same page or by a different instance of $jsonpCallbacks created by a different injector), it should not be re-initialized/overwritten.
|
LGTM |
|
Not related specifically to this PR: I wonder what would happen if someone overrode |
I am pretty sure it would 😃 If nothing else, we are using BTW, I just found out (accidentally while reviewing an unrelated PR) that |
| beforeEach(module(function($provide) { | ||
| // mock out the $window object | ||
| $provide.value('$window', { angular: {} }); | ||
| })); |
There was a problem hiding this comment.
There is already code in angular-mocks that cleans up angular.callbacks. This isn't necessary.
|
@petebacondarwin , @Narretz , PTAL. |
|
So you removed your test... is that because it cannot actually test what you wanted to test? |
|
I didn't remove my test. I remove a |
|
OK, so I didn't scroll down far enough. I am sure it wasn't there when I looked a minute ago... |
|
OK, so still LGTM |
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix.
What is the current behavior? (You can also link to an open issue here)
When bootstrapping multiple apps on the same page, one could overwrite JSONP callbacks defined by others.
What is the new behavior (if this is a feature change)?
The callback paths are usnique across apps.
Does this PR introduce a breaking change?
No.
Please check if the PR fulfills these requirements
Docs have been added / updated (for bug fixes / features)Other information:
/cc @petebacondarwin