feat: Throw errors for URL paths without leading slashes, which will never match

[TLPcoder]

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

[paulmelnikow]

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.

[TLPcoder]

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.

[paulmelnikow]

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 conditionally().

[paulmelnikow]

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.

[paulmelnikow]

Congrats on a first PR to Nock! 🎉 🚀

[nockbot]

🎉 This PR is included in version 11.5.0 🎉

The release is available on:

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

Your semantic-release bot 📦🚀