fix: allow unmocked when providing literal search params. #1614
Conversation
mastermatt
force-pushed
the
fix/literal-search-allow-mocked
branch
from
93e06c7
to
faf417d
Compare
fixes: nock#1421 There was inconsistent behavior around how search params were handled when they were provided as part of a URI string to new Interceptors. The issue happen to come to light when `allowUnmocked` was set. The fix normalizes the behavior by refactoring the constructor of Interceptor to look for literal search params. If present, they're stripped from the `path` attribute and set via the `query` method. As part of the work, `Interceptor.query` was modified to accept `URLSearchParams` instances as valid input.
mastermatt
force-pushed
the
fix/literal-search-allow-mocked
branch
from
faf417d
to
a953db4
Compare
There was a problem hiding this comment.
Tests look great and code changes look good too.
I'd prefer to throw when the querystring is specified more than once, though perhaps adding a TODO comment alongsside the test of the current behavior is fine. Changing the behavior of multiple calls to query() is probably better accomplished in a follow-on PR.
| scope.done() | ||
| }) | ||
|
|
||
| test('multiple set query keys use the first occurrence', async t => { |
There was a problem hiding this comment.
Seems like using the last occurrence might make more sense… or perhaps it would be better to throw an error?
I've noticed that when there are multiple calls to reply(), one of them takes preference… and come to think of it, I forget which. Throwing an error seems nicer to me.
There was a problem hiding this comment.
I was thinking the same thing. Since any change would be a breaking change, I thought it best to just add coverage of current expected functionality. I'll create an issue to add a breaking change and throw instead.
There was a problem hiding this comment.
For what it's worth, I'm not sure if people are ever deliberately using the library that way. It's not a documented behavior. So I'm not sure if it's a breaking change or a feature.
If we can ship it in 11.0, so much the better!
|
🎉 This PR is included in version 11.0.0-beta.25 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
Just want to say that I so appreciate the great work that you're doing on the library, @mastermatt! It's really great to have someone to dig through this with! Your contributions have been careful and thoughtful and they're having a big impact on the library. Many thanks! 🙌 |
|
🎉 This PR is included in version 11.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
fixes: #1421 There was inconsistent behavior around how search params were handled when they were provided as part of a URI string to new Interceptors. The issue happen to come to light when `allowUnmocked` was set. The fix normalizes the behavior by refactoring the constructor of Interceptor to look for literal search params. If present, they're stripped from the `path` attribute and set via the `query` method. As part of the work, `Interceptor.query` was modified to accept `URLSearchParams` instances as valid input.
fixes: #1421 There was inconsistent behavior around how search params were handled when they were provided as part of a URI string to new Interceptors. The issue happen to come to light when `allowUnmocked` was set. The fix normalizes the behavior by refactoring the constructor of Interceptor to look for literal search params. If present, they're stripped from the `path` attribute and set via the `query` method. As part of the work, `Interceptor.query` was modified to accept `URLSearchParams` instances as valid input.
fixes: #1421 There was inconsistent behavior around how search params were handled when they were provided as part of a URI string to new Interceptors. The issue happen to come to light when `allowUnmocked` was set. The fix normalizes the behavior by refactoring the constructor of Interceptor to look for literal search params. If present, they're stripped from the `path` attribute and set via the `query` method. As part of the work, `Interceptor.query` was modified to accept `URLSearchParams` instances as valid input.
fixes: #1421
There was inconsistent behavior around how search params were handled
when they were provided as part of a URI string to new Interceptors.
The issue happen to come to light when
allowUnmockedwas set.The fix normalizes the behavior by refactoring the constructor of
Interceptor to look for literal search params. If present, they're
stripped from the
pathattribute and set via thequerymethod.As part of the work,
Interceptor.querywas modified to acceptURLSearchParamsinstances as valid input.