Savory / Danet

The most mature backend framework for Deno. Create awesome HTTP and WebSocket server as well as KVQueue workers !
https://danet.land
Other
269 stars 18 forks source link

Feat: Allow query params to return all values #48

Closed rgoupil closed 2 years ago

rgoupil commented 2 years ago

Description

It is currently impossible to read all the values passed for a query param when it is present more than once, only the last one is kept.

For instance when for the following query ...?param=1&param=2:
@Query('param') would return "2"
@Query() would return { "param": "2" }

This PR introduce a @QueryAll decorator that is similar @Query except it always return all the values instead of only the last one in the URL. Using the same example as earlier:
@QueryAll('param') would return ["1", "2"]
@QueryAll() would return { "param": ["1", "2"] }

I also slightly changed the way @Param() get the path parameters (it's untyped but I swear that's how you're supposed to do it) in order to not return query params as well (which could create some conflict issues). While technically a breaking change, using @Param() to return the query params is most likely a terrible idea that I have never seen in any other frameworks.


Type of change


Checklist:

Sorikairox commented 2 years ago

Query and QueryAll are redundant IMO. There is no valid use-case where getting the last one with Query instead of using QueryAll is valid.

QueryAll should be renamed Query and if it takes no parameters it returns the same thing as Query without parameters.

Param does exist 😛 Check out https://docs.nestjs.com/controllers#full-resource-sample example 😄

rgoupil commented 2 years ago

Query and QueryAll are redundant IMO. There is no valid use-case where getting the last one with Query instead of using QueryAll is valid.

QueryAll should be renamed Query and if it takes no parameters it returns the same thing as Query without parameters.

Param does exist 😛 Check out https://docs.nestjs.com/controllers#full-resource-sample example 😄

I know Param exists 😛 but in the current state it returns the exact same object than Query, which is not right since Query is for query parameters and @Param is for path parameters! For reference, Nest gives these values: @Query(key?: string) | req.query / req.query[key] @Param(key?: string) | req.params / req.params[key]

You can see that Query and Param are not supposed to return the same value 😉

Sorikairox commented 2 years ago

Oh yes, you are a right, Query and Param returning the same value is a bug (that you fixed) !

I misinterpreted While technically a breaking change, using @Param() to return the query params is most likely a terrible idea that I have never seen in any other frameworks. Mybad !

rgoupil commented 2 years ago

Query and QueryAll are redundant IMO. There is no valid use-case where getting the last one with Query instead of using QueryAll is valid.

QueryAll should be renamed Query and if it takes no parameters it returns the same thing as Query without parameters.

I'll try to make a better job at explaining what QueryAll does, although it probably show that we should change it's name. Using ...?test=toto&myparam=1&myparam=2 as an example URL:

The idea behind not doing exactly what Nest is doing here is because the behaviour of their Query is really frustrating to work with: if your query parameter is only present once, then it return a string, if it is present multiple times then it return an array of string. This mean that the return type can differ quite a lot and you must always declare your variable as string | string[] and have an ugly bit of code to handle that it could be both. In my experience many developers don't know about this and end up only typing for string until their code break when they receive an array 😅

Please note that the behaviour of Query was not changed in any other way than not returning the path params anymore (which was a bug too 😋).