search

prototypejs / prototype

Prototype JavaScript framework
http://prototypejs.org/
Other
3.54k stars 639 forks source link

HTTP PUT/PATCH Support, Bodies? #280

Closed rye closed 7 years ago

rye commented 9 years ago

I use exotic/standard HTTP methods, including PUT and PATCH. Currently, these aren't supported by Prototype. I think it'd be great if PUT and PATCH were both supported, since they're standard functions. They function similarly to POST requests, but instead involve updates/changes to things instead of creation and fetching, which are what POST and GET are designed for, respectively.

To support, I'd recommend that the Ajax.Request options object no longer need the postBody attribute and instead consume a solid body attribute. Since GET requests can have a body, too (and do, in my use-case), it'd be great if this was just always sent off.

I'm willing to help but I'm new to the project. Anyone got any ideas on how to make this happen? For now I'll just use core XHR's.

fntz commented 9 years ago

For start read https://github.com/sstephenson/prototype#contributing-to-prototype and http://prototypejs.org/contribute

For add a new http request methods in Ajax need change this line https://github.com/sstephenson/prototype/blob/master/src/prototype/ajax/request.js#L186

But seems PATCH method not supported in IE < 10. Seems other http methods (post, get, delete, put) supported in all browsers

savetheclocktower commented 9 years ago

This is the difficulty we've had in the past. We support browsers all the way back to IE 6, and we've been reluctant to offer features that can't be made uniform across those browsers. We may have to revisit this philosophy.

savetheclocktower commented 9 years ago

To elaborate: I think we should give people the option of specifying more exotic HTTP verbs and trusting them to know whether to use them, as long as it's explained somewhere in our docs.

I'm also in favor of aliasing postBody to body and preferring that in the future. Not thrilled with the idea that a GET request can have a body, but if XHR allows it I don't see why we should stand in the user's way.

@fntzr's advice is good. One thing, @rye: if you're planning to submit a PR for this, I'd recommend working off of the mocha branch, rather than master. We're just about done overhauling our test suite and I'd hate to have you writing tests in a framework that's about to be obsoleted.

savetheclocktower commented 9 years ago

Also, consider our behavior where we change any HTTP verb that isn't GET or POST into a _method URL param: that's worth keeping around because it's convention that, e.g., some versions of Rails rely on. And it at least allows for a server to support older browsers that do not support those HTTP verbs. But maybe that behavior can be controlled by an option that is on by default but can be turned off for specific requests.

rye commented 9 years ago

@savetheclocktower, should Ajax requests be treated any differently if they're GETs or POSTs (or PUTs/PATCHs/UPDATEs/DELETEs)? The general flow that I've always seen when using XMLHttpRequests is as follows:

var xhr = new XMLHttpRequest();
xhr.open(<method string>, <url>);
xhr.onreadystatechange = <callback that does error handling and such>;
xhr.send(<body>);

This approach hardly ever changes between the different HTTP request types, since I think the XHR Web API handles discrepancies before blindly sending things off (e.g. if a request body will seem to cause server breakage). I may be wrong about that, though.

Since all HTTP/1.1 methods seem to be able to be given a body, we can just send off the body property if it's in the options.

I suppose the IE support thing may blockade certain types of requests, but we could just treat all methods fairly and act as a middleman by sending things off without complaint. If PUT isn't supported by the browser, then it's not. All that it seems that one needs to do is to handle the case where it isn't, and still be able to do everything else.

savetheclocktower commented 9 years ago

should Ajax requests be treated any differently if they're GETs or POSTs (or PUTs/PATCHs/UPDATEs/DELETEs)?

Probably not, but there are a couple places where they are treated differently: once before we send the request, and once when we build the response. (Plus the _method hack that we've already discussed.)

I suppose the IE support thing may blockade certain types of requests, but we could just treat all methods fairly and act as a middleman by sending things off without complaint. If PUT isn't supported by the browser, then it's not. All that it seems that one needs to do is to handle the case where it isn't, and still be able to do everything else.

Ajax requests are tricker than other things because you can't do capability checks. With the DOM, for instance, we know that certain browsers have certain quirks, but we can detect those quirks programmatically through tests, and we do so often in our code. Check out dom/dom.js for a bunch of examples.

We can't do similar sorts of capability checks with XHR — not if it means requesting an arbitrary URL every time prototype.js loads.

That's the rock, and here is the hard place: we have existing behavior with Ajax requests that must be preserved. At times we've broken backward compatibility out of necessity, but we don't do it when we have a choice not to. Which means that it'll be tough to rework the two code sections I pointed out earlier while preserving backward compatibility.

For the _method thing, at least, there's a way forward. Existing code that uses Prototype will turn a PUT request into _method=put in the URL, and whether that's proper or improper, it needs to stay that way. When a user updates Prototype, that existing code they wrote should work the same way by default.

My preferred way to make this work would be to create an option named (e.g.) simulateHTTPMethods which would default to true, but which the user could turn off, either for one request…

new Ajax.Request('/foo', {
  method: 'patch',
  simulateHTTPMethods: false
  // ...
});

…or for all requests. (The latter option would require a place to set default options for Ajax requests — something we probably should've had by now, but don't. Doesn't matter; it can be a per-request option to start.)

qsiebers commented 7 years ago

I've made a patch for this in the past for our product. It's only a code change without test cases, so it might require some work. I'll open a PR for it, so you can have a look at it and maybe use it as a starting point.