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

WIP: Make Queries use GET #888

Closed
wants to merge 5 commits into from
Closed

WIP: Make Queries use GET #888

wants to merge 5 commits into from

Conversation

shayneczyzewski
Copy link
Sponsor Contributor

@shayneczyzewski shayneczyzewski commented Dec 20, 2022

Description

This PR is open mainly for the discussion around shifting Queries from POST to GET. The approach in this PR does move us in the "right direction" of usingGET for their semantically correct operations. However, because Wasp lacks the input arg type info, if we simply made them URL params they would all come out on the server as strings. That would not be ideal since it would (a) be a breaking change, and (b) the user would have to do a ton of type conversions in their query functions. The approach taken here mirrors that of POST before- namely, serializing JSON as an arg. This is not ideal/traditional REST-style, however. :/

The main question is: do we wait on this until we allow users to specify the route and param types for Queries, or is this already a useful step in the right direction we want to take on, knowing it still may "look weird" to devs.

Before:

Request URL: http://localhost:3001/operations/get-task
Request Method: POST
Request Payload:
{"id":23}

After:

Request URL: http://localhost:3001/operations/get-task?args=%7B%22id%22:23%7D
Request Method: GET

Let's discuss! :D

Fixes #445

Type of change

Please select the option(s) that is more relevant.

  • Code cleanup
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Copy link
Member

@Martinsos Martinsos left a comment

Choose a reason for hiding this comment

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

Looks good @shayneczyzewski !

This is a bit hard decision. What are pros and cons?
I guess this makes it a bit nicer / more semantically correct, right?
Yes it is still one big argument, but ok.
If there wasn't for limit, I would say it is certainly better. But if we say limit is not a problem, then that also stands.
So I think it is ok to do this change, why not!

And in the future we can type it better. But I don't see anything very bad with having one big argument, to be honest.

waspc/data/Generator/templates/server/src/app.js Outdated Show resolved Hide resolved
Comment on lines -7 to -14
{=! TODO: When generating express route for query, generated code would be most human-like if we
generated GET route that uses query arguments.
However, for that, we need to know the types of the arguments so we can cast/parse them.
Also, there is limit on URI length, which could be problem if users want to send some bigger
JSON objects or smth.
So for now we are just going with POST that has JSON in the body -> generated code is not
as human-like as it should be though. =}
const args = req.body || {}
Copy link
Member

Choose a reason for hiding this comment

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

Ah yes well this captures it all!
Should we leave some of this comment around? Or maybe best just to create a GH issue to track this?

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

If we do go with this approach I will do one of those options for sure 👍🏻 thnx

@shayneczyzewski
Copy link
Sponsor Contributor Author

Thanks for your thoughts @Martinsos. The more I think about this, the more I am inclined to leave it as a POST until we have enough information about args to make them proper GET requests. Right now, we are just trading one "strange" thing for another, which probably isn't worth it. And while the 2k char limit for some browsers for URIs is also there, I think that is a side concern- if you are adding that much info to your GET request I think "you are doing it wrong." :D

What do you think @sodic? Use this? Punt until we have proper types associated with route/query params in .wasp files? Something else? Thnx

@shayneczyzewski shayneczyzewski changed the title WIP: Make Queries use GET Discussion: Make Queries use GET Dec 21, 2022
@shayneczyzewski shayneczyzewski changed the title Discussion: Make Queries use GET WIP: Make Queries use GET Dec 21, 2022
@Martinsos
Copy link
Member

Makes sense! I am ok with both, although if I had to choose right now, I would probably go with this GET even though it has "weird" arguments. It still feels a bit more correct. But whatever you decide I am good with!

@sodic
Copy link
Contributor

sodic commented Dec 23, 2022

Punt until we have proper types associated with route/query params in .wasp files?

Yes, I prefer sticking with POST for now.

I know some sites use long base64 URLs, but I always found it ugly. It's also a non-standard approach (i.e., browsers and the infrastructure aren't built to optimize on it). Plus, I think it makes for a worse developer experience (think about how debugging requests would look like in Chrome DevTools, or even by reading the logs).

Therefore, I agree with Shayne's last comment, we would be substituting one strange thing for another, and the one we have now is at least human readable and easy to debug.

@Martinsos
Copy link
Member

All right, sounds good to me!

@shayneczyzewski
Copy link
Sponsor Contributor Author

Thanks for the input @Martinsos and @sodic. I will close this PR now. At least we reminded ourselves of the reasons to stick with POST until we have proper types for operations. :) One day... haha

@Martinsos
Copy link
Member

Hah yes :D. Maybe sooner than we think :D!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should we consider using GET instead of POST for Queries?
3 participants