fix(http):fix jsonp callbackname is undefined by doxiaodong · Pull Request #14270 · angular/angular · GitHub

fix #14267

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. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

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.

revert

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

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

@alxhub this pr will add callbacks to the global namespace (window). It may be very unexpected :)
@mhevery said he will revert 9e5617e (commit that introduced the bug)
and it's already fixed in 4.0 so I don't think you should merge this

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
so I think you can close this

@doxiaodong What is the status of the task? @angular/http is deprecated, please closed PR

ok

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

googlebot added the cla: no label Feb 3, 2017

doxiaodong force-pushed the jsonp branch 2 times, most recently from b670a34 to 471a8e6 Compare February 3, 2017 09:14

googlebot added cla: yes and removed cla: no labels Feb 3, 2017

doxiaodong force-pushed the jsonp branch from 471a8e6 to cebf75b Compare February 3, 2017 10:00

fix(http):fix jsonp callbackname is undefined

a035347

doxiaodong force-pushed the jsonp branch from cebf75b to a035347 Compare February 3, 2017 10:08

Urigo mentioned this pull request Feb 3, 2017

Angular-GraphQL cookbook angular/angular.io#3135

Open

9 tasks

doxiaodong force-pushed the jsonp branch from 4c1d21f to 19778c5 Compare February 3, 2017 13:48

http: add jsonp callback name test

4a7e627

doxiaodong force-pushed the jsonp branch from 19778c5 to 4a7e627 Compare February 3, 2017 14:00

DzmitryShylovich reviewed Feb 3, 2017

View reviewed changes

alxhub self-requested a review February 10, 2017 20:20

alxhub approved these changes Feb 10, 2017

View reviewed changes

chuckjaz added the comp: http label May 23, 2017

doxiaodong closed this Mar 1, 2018

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 13, 2019

                 nextRequestID(): string { return `__req${_nextRequestId++}`; }
-                requestCallback(id: string): string { return `${JSONP_HOME}${id}_finished`; }
+                requestCallback(id: string): string { return _getJsonpCallbackName(id); }

                   });
-                  it('callback name should not contain dots', () => {
+                  it('callback name should be a function name', () => {

fix(http):fix jsonp callbackname is undefined by doxiaodong · Pull Request #14270 · angular/angular · GitHub

Uh oh!

fix(http):fix jsonp callbackname is undefined - #14270

fix(http):fix jsonp callbackname is undefined#14270doxiaodong wants to merge 2 commits intoangular:masterangular/angular:masterfrom doxiaodong:jsonpdoxiaodong/angular:jsonpCopy head branch name to clipboard

Conversation

doxiaodong commented Feb 3, 2017

Uh oh!

googlebot commented Feb 3, 2017

Uh oh!

googlebot commented Feb 3, 2017

Uh oh!

DzmitryShylovich commented Feb 3, 2017

Uh oh!

doxiaodong commented Feb 3, 2017

Uh oh!

DzmitryShylovich Feb 3, 2017

Choose a reason for hiding this comment

Uh oh!

DzmitryShylovich Feb 3, 2017

Choose a reason for hiding this comment

Uh oh!

doxiaodong Feb 3, 2017

Choose a reason for hiding this comment

Uh oh!

DzmitryShylovich commented Feb 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alxhub commented Feb 10, 2017

Uh oh!

DzmitryShylovich commented Feb 10, 2017

Uh oh!

doxiaodong commented Feb 15, 2017

Uh oh!

doxiaodong commented Feb 20, 2017

Uh oh!

DzmitryShylovich commented Feb 21, 2017

Uh oh!

splincode commented Mar 1, 2018

Uh oh!

doxiaodong commented Mar 1, 2018

Uh oh!

angular-automatic-lock-bot Bot commented Sep 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

fix(http):fix jsonp callbackname is undefined#14270
doxiaodong wants to merge 2 commits into
angular:masterfrom
doxiaodong:jsonp

DzmitryShylovich commented Feb 10, 2017 •
edited
Loading