fix: make sure array search params using different syntaxes are supported #4148
Conversation
dario-piotrowicz
force-pushed
the
dario/multi-search-params-normalization
branch
2 times, most recently
from
97cee8d to
6d9cb9d
Compare
lib/mock/mock-utils.js
Outdated
| @@ -103,8 +135,7 @@ function safeUrl (path) { | |||
| return path | |||
| } | |||
|
|
|||
| const qp = new URLSearchParams(pathSegments.pop()) | |||
| qp.sort() | |||
| const qp = normalizeQueryParams(pathSegments.pop()) | |||
There was a problem hiding this comment.
Can you put this behind an option? This behavior is non-standard.
There was a problem hiding this comment.
of course, I will look into doing that 🙂👍
my reasoning was that this is more of additive support that allows the other syntaxes without taking anything away from people not caring about/using them, but I guess that it'd be cleaner not to include them by default more from a spec-compliant/correctness point of view? 🤔
There was a problem hiding this comment.
I've put the logic in MockAgent under a new acceptNonStandardSearchParameters option, please have a look and let me know how this looks 🙏
(PS: I still need to add this to the docs, but before doing that I wanted to check with you to see if I am heading to the right direction 🙂)
| @@ -59,6 +59,9 @@ declare namespace MockAgent { | |||
| /** Ignore trailing slashes in the path */ | |||
| ignoreTrailingSlash?: boolean; | |||
|
|
|||
| /** Accept URLs with search parameters using non standard syntaxes. default false */ | |||
| acceptNonStandardSearchParameters?: boolean; | |||
There was a problem hiding this comment.
Can you add a test for this type? We use tsd.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
sure, sorry I missed that 🙂👍
There was a problem hiding this comment.
Co-authored-by: Carlos Fuentes <[email protected]>
…meters` option remove unnecessary normalization
dario-piotrowicz
force-pushed
the
dario/multi-search-params-normalization
branch
from
cf7daab to
2eb2cb1
Compare
Rationale
This PR adds support for the
MockAgentto intercept requests such ashttps://example.com/qux-c?arraykey[]=a&arraykey[]=bChanges
Requests such as
https://example.com/qux-c?arraykey[]=a&arraykey[]=bget converted into "normalized" ones likehttps://example.com/qux-c?arraykey=a&arraykey=bso that they can then be handled normallyFeatures
N/A
Bug Fixes
Fixes #4146
Breaking Changes and Deprecations
Some requests that before wouldn't get intercepted would be now, this could be considered a breaking change 🤔 but maybe not since those should have been intercepted in the first place? 🤔
Status
Note
My implementation fixes the case of array values having
[](e.g.a[]=1,a[]=2,a[]=3) and also different array values separated by commas (e.g.a=1,2,3) and that's pretty much it, it could potentially support other syntaxes like the JSON API and Bitbucket API mentioned in https://medium.com/raml-api/arrays-in-query-params-33189628fa68, but that feels to me like quite an overkill right now. I am thinking that probably the cases I am adding support for now should cover most use cases anyways and if the extra ones will be needed in the future (someone opening an issue, etc...) they can always be incrementally added as needed on top of my changes 🤔