Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

feat($http): setting JSONP callback name #3073

Closed
wants to merge 2 commits into from

Conversation

ricardohbin
Copy link
Contributor

Allow to define a name to angular.callbacks function. It would be useful to caching (varnish, etc).

Ex:

$http.jsonp("http://example.com?callback=JSON_CALLBACK(myFunction)").success(...)

Will always request

http://example.com?callback=angular.callbacks.myFunction

@petebacondarwin
Copy link
Member

@IgorMinar - is there a security issue around using the same callback name repeatedly? Or is the current design just to ensure that multiple requests do not accidentally land on each others callback functions?

@petebacondarwin
Copy link
Member

PR Checklist (Minor Feature)

  • Contributor signed CLA now or in the past (if you just signed, leave a comment here with your real name for cross reference)
  • Feature improves existing core functionality
  • API is compatible with existing Angular apis and relevant standards (if applicable)
  • PR doesn't contain a breaking change
  • PR contains unit tests
  • PR contains e2e tests (if suitable)
  • PR contains documentation update
  • PR passes all tests on Travis (sanity)
  • PR passes all tests on ci.angularjs.org (cross-browser compatibility)
  • PR is rebased against recent master
  • PR is squashed into one commit per logical change
  • PR's commit messages are descriptive and allows us to autogenerate release notes (required commit message format)
  • All changes requested in review have been implemented

@petebacondarwin
Copy link
Member

@ricardohbin - can you please sign the CLA? Thanks

@ricardohbin
Copy link
Contributor Author

@petebacondarwin I already signed it in other PR. :)

@IgorMinar
Copy link
Contributor

rather than hardcoding the callback name, can't we just generate it in a repeatable and deterministic way based on the url requested?

that way things can keep on working just as they work today but the callback name will be reused if the url is the same.

this does however mean that we need to be able to handle a situation when two jsonp requests are made for the same url at the same time. the expected result should be that we'll call the callbacks in the order their requests were made.

so internally we should have a map of stableCallbackIds to arrays instead of randomCallbackIds to callbacks.

@jwagner
Copy link

jwagner commented Aug 23, 2013

@IgorMinar that could lead to some slightly odd behavior though:
$http.jsonp(url).success(callbackA);
$http.jsonp(url).success(callbackB);
Could result in CallbackA being called with the response of CallbackB. While I'd agree that relying on that kind of ordering of request wouldn't be a good idea in practice it'd still add a potential stumbling block.

I guess a potential solution could be to use a counter if there are parallel requests in flight, but that wouldn't be very pretty either.

@cm325
Copy link

cm325 commented Sep 4, 2013

Having two or more requests in single mode is a problem with this patch. As a temporary way to take advantage of this, without going down the SHA route (which would be great), we can mimic a dynamic callback name by providing the defaultParams with an "@callback" and provide the callback name from a parameter, like:

  SiteImage.query({
              find:"byMostRecentAndId",
              siteId:siteDescriptions[i].locationId,
              callback:"JSON_CALLBACK(siteimage" + siteDescriptions[i].locationId + ")"
            }, etc

as long as the params are unique, then you don't get caching collisions-

@revolunet
Copy link
Contributor

Related issue : the callback name itself.

The BING jsonp api for example prevent the use of dots in name.

What about being able to override the default window.angular.callbacks global in the config phase ?
$httpProvider.setJsonpCallbacksPrefix('myGlobalCallbackMap');

var callbackId = '_' + (callbacks.counter++).toString(36);
var callbackId = (/JSON_CALLBACK\((.*)\)/).exec(url) || ('_' + (callbacks.counter++).toString(36));

if (callbackId.constructor.name === "Array") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would reword it more like this:

var base = (/JSON_CALLBACK\((.*?)\)/).exec(url);
base = base ? base[1] : '';
callbackId = base + ('_' + (callbacks.counter++).toString(36));

This way we use the nam

callbacks[callbackId] = function(data) {
callbacks[callbackId].data = data;
};

var jsonpDone = jsonpReq(url.replace('JSON_CALLBACK', 'angular.callbacks.' + callbackId),

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trailing whitespace

@caitp
Copy link
Contributor

caitp commented Oct 27, 2014

Obviously this has been bitrotten for a pretty long time, but I don't see a huge problem with it really

@ricardohbin
Copy link
Contributor Author

updating the fork ;)

@pixelcort
Copy link

I'm having the issue with an API that doesn't support dots. Perhaps a related solution is to change to callbacks that don't contain dots.

@caitp
Copy link
Contributor

caitp commented Nov 10, 2014

We would then be polluting global variables --- this is not good. The right thing to do is either 1) don't use jsonp at all, if you can avoid it, or 2) pressure your API provider to work better

@revolunet
Copy link
Contributor

  1. meanwhile, use any kind of proxy, eg http://www.corsproxy.com

@katzlbt
Copy link

katzlbt commented Apr 23, 2015

The pyramid web framework disallows angular callbacks since newer versions.
https://pyramid.readthedocs.org/en/master/_modules/pyramid/renderers.html
JSONP_VALID_CALLBACK = re.compile(r"^[a-zA-Z_$][0-9a-zA-Z_$]+$")

@pixelcort
Copy link

Still have an API that's not allowing dots; I know polluting global namespace is bad but right now the Angular feature can't be used because of this. An alternative is some flag to remove the dots.

@petebacondarwin
Copy link
Member

I think this is annoying enough - and might require a breaking change - to be highlighted for 1.5

@petebacondarwin
Copy link
Member

Could we alleviate this by moving the callback stuff into its own service, say $jsonpCallbacks, that developers can override.

The service could have two methods getCallback(url : string?) : string and getData(callback: string): object|array.

getCallback(url) would return a string describing the callback to be sent to the server. The built-in service would just return 'angular.callbacks.' + id. Where id is computed just as in the current $httpBackend service.

getData(callback) would return the parsed JSON data for the the given callback.

@petebacondarwin petebacondarwin self-assigned this Jun 6, 2016
petebacondarwin added a commit to petebacondarwin/angular.js that referenced this pull request Jun 17, 2016
Use the built-in service to handling callbacks to `$http.jsonp` requests.

Closes angular#3073
petebacondarwin added a commit to petebacondarwin/angular.js that referenced this pull request Jun 18, 2016
Use the built-in service to handling callbacks to `$http.jsonp` requests.

Closes angular#3073
petebacondarwin added a commit to petebacondarwin/angular.js that referenced this pull request Jun 18, 2016
Use the built-in service to handling callbacks to `$http.jsonp` requests.

Closes angular#3073
@petebacondarwin
Copy link
Member

@ricardohbin, @revolunet, @pixelcort, @katzlbt - please take a look at #14795
Closing this PR in favour of that one.

@rajeshmanepalli
Copy link

hi,

please unsubscribe my mailid

On Sat, Jun 18, 2016 at 10:06 PM, Pete Bacon Darwin <
notifications@github.com> wrote:

Closed #3073 #3073.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#3073 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/ADvOtau3CW3fOez7ExZpTGj8Pnyvki4Dks5qNB5-gaJpZM4AxM1c
.

petebacondarwin added a commit that referenced this pull request Jun 21, 2016
Use the built-in service to handling callbacks to `$http.jsonp` requests.

Closes #3073
Closes #14795
petebacondarwin added a commit that referenced this pull request Jun 21, 2016
Use the built-in service to handling callbacks to `$http.jsonp` requests.

Closes #3073
Closes #14795
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet