feat(interceptor): duplicate query keys throw

[mastermatt]

Resolves #1623

BREAKING CHANGE: Providing a duplicate search parameter to the query
method throws an error instead of ignoring subsequent values.

[gr2m]

This is great 👏

[nockbot]

🎉 This PR is included in version 11.0.0-beta.26 🎉

The release is available on:

• npm package (@beta dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[paulmelnikow]

Hmm, I was thinking about this a little differently. I was thinking we'd throw if there were multiple attempts to define the query, regardless of whether the keys duplicated each other. Meaning you would get an error if you did .get('/?foo=bar').query({ myQs }) regardless of what's inside myQs. The idea being that is probably a mistake; with one or the other of those going unnoticed.

Or .get('/').query().query(), which probably also is a mistake.

What do you think?

[gr2m]

Hmm sounds reasonable. Even if folks might want to merge query options conditionally, they can do that before passing it to .query(). I’d be okay to throw an error if .query() is called more than once

[mastermatt]

I'm in favor of throwing an error if trying to set query filter data twice, but if we go that route I have two questions:

Should we allow the user to explicitly unset query filtering?
Something like .get(somePathVarThatMightHaveAQuery).query(false) for those cases where users are passing a path var around?

Second: we still need to handle duplicate query keys in some way.
The more I poke around with options, the more I think we have a bug that this PR changed, but didn't fix.
Note that .get('/?foo=one&foo=two') is a valid query and we should probably support it.
I'm thinking that updating the Interceptor.queries attribute from an object to be an array using the "entities" format [[key, value], ...] will be the right approach. That will mean updating the match method too.

[paulmelnikow]

Something like .get(somePathVarThatMightHaveAQuery).query(false) for those cases where users are passing a path var around?

I think it's not necessary. Nock has so much API surface and if it comes to it they can somePathVarThatMightHaveAQuery.split('?')[0] easily enough.

Note that .get('/?foo=one&foo=two') is a valid query and we should probably support it.

Right! Though since we were silently dropping duplicates before, I figured an explicit error was no worse…

I'm thinking that updating the Interceptor.queries attribute from an object to be an array using the "entities" format [[key, value], ...] will be the right approach. That will mean updating the match method too.

Yes. That makes sense to me. 👍

URLSearchParams and literal query strings would be two ways of specifying queries with duplicated keys.

[nockbot]

🎉 This PR is included in version 11.0.0 🎉

The release is available on:

• npm package (@next dist-tag)
• GitHub release

Your semantic-release bot 📦🚀