fix(http):fix jsonp callbackname is undefined #14270
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
b670a34 to
471a8e6
Compare
|
CLAs look good, thanks! |
|
need to add a test to ensure that callback is actually executed to prevent such situations in the future |
|
I update the callback name test, it can be any function name. |
| @@ -32,17 +30,13 @@ export class BrowserJsonp { | |||
|
|
|||
| nextRequestID(): string { return `__req${_nextRequestId++}`; } | |||
|
|
|||
| requestCallback(id: string): string { return `${JSONP_HOME}${id}_finished`; } | |||
| requestCallback(id: string): string { return _getJsonpCallbackName(id); } | |||
| @@ -70,10 +70,11 @@ export function main() { | |||
| expect(instance).toBeAnInstanceOf(JSONPConnection); | |||
| }); | |||
|
|
|||
| it('callback name should not contain dots', () => { | |||
| it('callback name should be a function name', () => { | |||
There was a problem hiding this comment.
as we saw testing callback name doesn't actually verify anything.
Need a test for Uncaught ReferenceError: __ng_jsonp____req0_finished is not defined arror
There was a problem hiding this comment.
this callback name can be any validate function name, and I think it's enough. What to cause __ng_jsonp____req0_finished is not defined is the error of the data https://github.com/angular/angular/blob/master/modules/playground/src/jsonp/people.json#L2
|
I actually think it's okay, and the behavior is correct. The callbacks are only there while the request is in progress, and some APIs have issues with dots in the callback names. |
|
ok :) |
|
so, If I need to resolve the conflicts? |
|
how about this? |
|
@doxiaodong it will be fixed in the new http module in 4.0 |
|
@doxiaodong What is the status of the task? |
|
ok |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
fix #14267