Skip to content

Commit

Permalink
fix(common): canceled JSONP requests won't throw console error with m…
Browse files Browse the repository at this point in the history
…issing callback function (#36807)

This commit fixes a use-case where unsubscribing from a JSONP request will result in "Uncaught ReferenceError: ng_jsonp_callback_xy is not defined"
thrown into console. Unsubscribing won't remove its associated callback function because the requested script will finish
loading anyway and will try to call the handler.

PR Close #34818

PR Close #36807
  • Loading branch information
martinsik authored and thePunderWoman committed Feb 24, 2022
1 parent d388522 commit 64da1da
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 6 deletions.
14 changes: 9 additions & 5 deletions packages/common/http/src/jsonp.ts
Expand Up @@ -125,15 +125,17 @@ export class JsonpClientBackend implements HttpBackend {
// cleanup() is a utility closure that removes the <script> from the page and
// the response callback from the window. This logic is used in both the
// success, error, and cancellation paths, so it's extracted out for convenience.
const cleanup = () => {
const cleanup = (removeCallback = true) => {
// Remove the <script> tag if it's still on the page.
if (node.parentNode) {
node.parentNode.removeChild(node);
}

// Remove the response callback from the callbackMap (window object in the
// browser).
delete this.callbackMap[callback];
if (removeCallback) {
// Remove the response callback from the callbackMap (window object in the
// browser).
delete this.callbackMap[callback];
}
};

// onLoad() is the success callback which runs after the response callback
Expand Down Expand Up @@ -218,7 +220,9 @@ export class JsonpClientBackend implements HttpBackend {
node.removeEventListener('error', onError);

// And finally, clean up the page.
cleanup();
// Unsubscription won't remove the callback handler because there's no way to stop
// loading <script> once it has been added to DOM.
cleanup(false);
};
});
}
Expand Down
6 changes: 5 additions & 1 deletion packages/common/http/test/jsonp_spec.ts
Expand Up @@ -17,7 +17,6 @@ function runOnlyCallback(home: any, data: Object) {
const keys = Object.keys(home);
expect(keys.length).toBe(1);
const callback = home[keys[0]];
delete home[keys[0]];
callback(data);
}

Expand Down Expand Up @@ -63,6 +62,11 @@ const SAMPLE_REQ = new HttpRequest<never>('JSONP', '/test');
});
document.mockError(error);
});
it('allows the callback to be invoked when the request is cancelled', () => {
backend.handle(SAMPLE_REQ).subscribe().unsubscribe();
runOnlyCallback(home, {data: 'This is a test'});
expect(Object.keys(home).length).toBe(0);
});
describe('throws an error', () => {
it('when request method is not JSONP',
() => expect(() => backend.handle(SAMPLE_REQ.clone<never>({method: 'GET'})))
Expand Down

0 comments on commit 64da1da

Please sign in to comment.