Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move jsonp to its own method? #1068

Closed
lhorie opened this issue May 24, 2016 · 6 comments
Closed

Move jsonp to its own method? #1068

lhorie opened this issue May 24, 2016 · 6 comments
Labels
Type: Breaking Change For any feature request or suggestion that could reasonably break existing code
Milestone

Comments

@lhorie
Copy link
Member

lhorie commented May 24, 2016

Totally bikeshedding, but what do you guys think about moving jsonp to its own method in v1.0?

Current behavior:

//xhr
m.request({url: url, method: "GET"})

//jsonp
m.request({url: url, dataType: "jsonp"})

Proposed:

//xhr
m.xhr({url: url, method: "GET"})

//jsonp
m.jsonp({url: url})
@lhorie lhorie added discussion Type: Breaking Change For any feature request or suggestion that could reasonably break existing code labels May 24, 2016
@dead-claudia
Copy link
Member

dead-claudia commented May 24, 2016

Works for me. Also, a plain string URL should be accepted for the common case IMO, using the "GET" for m.xhr.

//xhr
m.xhr({url: url, method: "GET"})
m.xhr({url: url})
m.xhr(url)

//jsonp
m.jsonp({url: url})
m.jsonp(url)

Although...isn't XHR moot with the Fetch API? Beyond a few infrequent cases like progress notifications and cancellability (not just "don't care" that's being proposed at WHATWG), what's the benefit?

@Naddiseo
Copy link
Contributor

  • +0.5 for moving jsonp its own method.
  • -0 for m.xhr, seems like an implementation detail that is leaking to the API. I think either keeping the current m.request, and/or having shortcut methods, such as m.get/m.post/m.json, allows the implementation to change later without the API changing.
  • +1 for allowing plain string argument.

@dead-claudia
Copy link
Member

@Naddiseo Leo is just considering separating the two methods completely. The two cases share deceptively little code in the first place, anyways.

As for the name, I'd prefer m.xhr to remain m.request, minus the JSONP knowledge. Keep in mind, though, as long as XMLHttpRequests are created and exposed via something like config, it's no longer an implementation detail. And if it's using fetch behind the scenes, it's still not just an implementation detail, because it'll need polyfilled.

@lhorie
Copy link
Member Author

lhorie commented Jun 1, 2016

They actually share a lot of code, but it just seems somewhat silly that the signature completely changes based on whether there's a dataType option. Also, the two are fundamentally different things under the hood

@dead-claudia
Copy link
Member

dead-claudia commented Jun 1, 2016

@lhorie Eh...I didn't remember all the common option handling until after I wrote that...

They share quite a bit of code, but it's the actual launching of the request that's very different. There's just a lot less to configure with JSONP than with XHRs.

@lhorie
Copy link
Member Author

lhorie commented Jun 2, 2016

Ok so I kept m.request for xhr and added m.jsonp for jsonp

@lhorie lhorie closed this as completed Jun 2, 2016
@dead-claudia dead-claudia added this to the Rewrite milestone Jun 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Breaking Change For any feature request or suggestion that could reasonably break existing code
Projects
None yet
Development

No branches or pull requests

3 participants