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

RESTController - Allow for GET requests #630

Closed
ridem opened this issue Aug 9, 2018 · 14 comments
Closed

RESTController - Allow for GET requests #630

ridem opened this issue Aug 9, 2018 · 14 comments
Assignees
Labels
type:feature New feature or improvement of existing feature

Comments

@ridem
Copy link

ridem commented Aug 9, 2018

Note: As originally proposed at #629

Motivations:

  • It gets very difficult to consider using the browser version of Parse JS SDK in a production environment, as the requests made make caching and optimization very difficult
  • Allowing for GET requests leverages on all the browser caching features (ETag & such) that parse-server is already serving. If ETags are served by parse-server, it surely was to be used in the web browser - the fact that the official JS SDK doesn't support it on its end is suprising
  • GET requests can be cached by a CDN service like Cloudfront - which would conveniently cache the requests at edge-level based on a few headers.

Drawbacks:

  • As was mentioned in the PR, many tests rely on this mapping, but I can rewrite them
  • Some current users may rely on POST requests being sent by the SDK, but this "non-POST" feature could be made optional
  • The authors surely had a good reason at the time to map back every request to a POST request. I'm curious to learn about them and to know if they still apply today.

If this was made to fix some concerns with the requests sent from a web browser (compatibility, security), I'm wondering why having bothered with parse-server sending browser-caching compliant headers.
If it's because it's just the way the requests should be sent to a parse-server, then I don't understand why the REST Guide suggests otherwise.

@flovilmart
Copy link
Contributor

I can't answer for everything but for etags, this was enabled there so you may want to ask the original author directly.

sending browser-caching compliant headers.

Besides etag I don't believe the server provides any cache headers.

then I don't understand why the REST Guide suggests otherwise.

The REST API is the main API used by the SDK, the fact that you can bundle your requests as a POST request is an implementation detail. If you build a native SDK, you should use the REST API.

I would be enclined to let the JS SDK have an optional Parse.preserveAPIMethods() or Parse.useHTTPHeaders() that would toggle the way the REST controller behaves.

ALSO, this can be implemented in a separate REST controller outside the SDK for now. So you can test it, improve it and understand the limits. Switching the RESTController is easy:

Parse.CoreManager.setRESTController(myCustomRESTController)

@ridem
Copy link
Author

ridem commented Aug 9, 2018

Besides etag I don't believe the server provides any cache headers.

True, just checked again, my bad

ALSO, this can be implemented in a separate REST controller outside the SDK for now.

Thanks a lot for the tip! That'll work for me for now!

I've been testing these GET requests today and it seems to work perfectly, even with complex queries.

Another interesting thing to note is that the Parse Dashboard's API Console itself sends proper GET/PUT/POST/DELETE requests from the web browser, and doesn't implement its requests with ._method:
https://github.com/parse-community/parse-dashboard/blob/master/src/dashboard/Data/ApiConsole/ApiConsole.react.js#L87
https://github.com/parse-community/parse-dashboard/blob/master/src/dashboard/Data/ApiConsole/request.js

I'm staying on the loop to make a PR if it's ever considered for being implemented, depending on the original authors' feedback.
Meanwhile your setRESTController trick, which I had missed, works for me.

@ridem
Copy link
Author

ridem commented Aug 9, 2018

One good reason I could think of is IE9 compatibility / cross domain requests at the time. Maybe sending headers was tricky with IE9, as the following code suggests:
https://github.com/parse-community/Parse-SDK-JS/blob/master/src/RESTController.js#L44
headers is passed to ajaxIE9, but ajaxIE9 only takes 3 arguments, and headers isn't implemented for it then.

@flovilmart
Copy link
Contributor

Ideally I'd love to abstract away all the request making of this SDK, and probably remove the IE9 support etc... when possible :)

@andrewimm
Copy link
Contributor

It's unrelated to IE9, but it is related to CORS.
My memory is a bit hazy, because I wrote all this code 3 years ago, and we had been doing the POST thing for 2+ years before that even... but I'm pretty sure it was to avoid preflight, which would cause two round trips to the server (OPTIONS + secondary verb).
Right now we have a "simple" request – its content type is "text/plain," the verb is one of GET/POST, and we have no extra headers. The final part is the critical bit. When we start sending custom headers across origins, the browser needs to request OPTIONS to ask which headers are allowed. With a simple request, we only send one request to the server and it should be faster.

@flovilmart
Copy link
Contributor

That would make total sense! Thanks @andrewimm.

@andrewimm
Copy link
Contributor

Also since the "headers" arg came up, I'm pretty sure the only time we do pass custom headers is related to uploading files. That one endpoint needed to support some extra behavior and go around the typical networking path.

@ridem
Copy link
Author

ridem commented Aug 14, 2018

Thanks a lot for your feedback, this makes a lot of sense.
I'm actually calling my Parse backend from the same domain (proxying /api to the backend instance), hence not needing to worry about the preflight requests.
I guess the status quo on this issue is good enough for now.
It would have been simpler for me to quickly set Parse.useHTTPHeaders(), but on the other hand, I've been able to plug my own RESTController that uses my frontend's fetch with headers using Parse.CoreManager.setRESTController. Maybe I can share this code somewhere here.

@andrewimm You're right, it is: https://github.com/parse-community/Parse-SDK-JS/blob/master/src/ParseFile.js#L244

@andrewimm
Copy link
Contributor

That was the entire goal of CoreManager in the first place – allowing pluggable implementations. We used a custom REST implementation in the hosted version of Cloud Code, too. Glad the architecture is still valuable.

@flovilmart
Copy link
Contributor

@andrewimm this is through a custom rest controller that the server provides a 'direct access' controller.

@flovilmart
Copy link
Contributor

@ridem do you mind if we close this one then?

@ridem
Copy link
Author

ridem commented Aug 14, 2018

I agree, let's close this issue!
Thanks a lot for your reactivity and feedback, @flovilmart @andrewimm

@ridem ridem closed this as completed Aug 14, 2018
@flovilmart
Copy link
Contributor

Always a pleasure!

@sunshineo
Copy link

This post is very useful! Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature New feature or improvement of existing feature
Projects
None yet
Development

No branches or pull requests

5 participants