feat: Throw errors for URL paths without leading slashes, which will never match#1744
feat: Throw errors for URL paths without leading slashes, which will never match#1744paulmelnikow merged 6 commits intonock:masterfrom TLPcoder:empty-string-1730
Conversation
…e uri param to the interceptor class
paulmelnikow
left a comment
There was a problem hiding this comment.
Hi, thanks for this! Here are a few minor comments.
Another question is whether we should consider this a new feature (i.e. a better error message when passing a string that will match nothing) or if there is any legitimate use case where an empty string (or no string) is passed.
I guess people using filteringScope() might be passing a garbage string because that has always been allowed. So I think this needs to be considered a breaking change.
We've discussed setting a release schedule and batching up some of these breaking changes (so we only ship a breaking release every few months). Let's sort that out before merging this.
|
To make this change the most backwards compatible I have added a check for the filteringScope option and have added a test in a newer commit with the changes you requested besides the startsWith change. If the filteringScope option is populated the '/' check will not be honored. Because the path will never be used if filteringScope option is populated I think it makes sense to ignore the '/' check or require no argument at all. I think the check for filteringScope should be include even if this commit is merged in the breaking change batch. |
I think this is a good solution, and means we can call this a new feature. Passing a string that never matches is almost certainly a programmer error, and Nock now throws an error when this happens. If a programmer wants a conditional match and used a non-matching string as a workaround, this is better done using |
|
This is looking good. I left a couple minor suggestions about test wording, though feel like this is otherwise good to merge as a new feature. |
|
Congrats on a first PR to Nock! 🎉 🚀 |
|
🎉 This PR is included in version 11.5.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Added a check for filteringScope in the scope options as @paulmelnikow suggested. Also updated the two test which did not have the root path defined as this change will enforce this. To make the compatible with request like { hostname: 'example.test' } changed default value from { path = '' } to { path = '/' } in the interceptor class.
Fix #1730