Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Throw errors when base URLs are specified incorrectly [WIP] #1692

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

paulmelnikow
Copy link
Member

  • Switch to the new URL API which throws errors for most invalid inputs
  • Throw errors for URLs with unrecognized schemes
  • Add tests for cases that previously had been handled incorrectly

Closes #1061
Supersedes #1065

This is arguably a breaking change as it throws errors in cases where the previous code would no-op. How we consider it is related to the discussion in #1673.

@paulmelnikow
Copy link
Member Author

paulmelnikow commented Aug 22, 2019

Interesting failure. While url.parse('http://[2607:f0d0:1002:51::4]:8080').hostname drops the brackets and produces '2607:f0d0:1002:51::4', new URL('http://[2607:f0d0:1002:51::4]:8080').hostname keeps the brackets and produces '[2607:f0d0:1002:51::4]'.

It looks like Nock is concatenating the port onto the unbracketed IPv6 addresses, which seems less ideal:

connect EHOSTUNREACH 2607:f0d0:1002:51::4:8080 - Local (:::64394)

I don't think this has a functional effect, though aesthetically it seems like we should put the brackets in there. It improves readability of the error messages.

That requires a change in the way matching happens – we need to re-bracket options.host.

A separate adjustment probably is needed to preserve backward compatibility of IPv6 recordings in the current format.

this.port = null
this.parsedUrl = {}
} else {
this.parsedUrl = new URL(basePath)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

- Switch to the new URL API which throws errors for most invalid inputs
- Throw errors for URLs with unrecognized schemes
- Add tests for cases that previously had been handled incorrectly

Closes #1061
Supersedes #1065
@paulmelnikow paulmelnikow changed the base branch from next to master September 6, 2019 19:44
@paulmelnikow paulmelnikow changed the title fix: Throw errors when base URLs are specified incorrectly fix: Throw errors when base URLs are specified incorrectly [WIP] Sep 6, 2019
@stale
Copy link

stale bot commented Dec 5, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We try to do our best, but nock is maintained by volunteers and there is only so much we can do at a time. Thank you for your contributions.

@gr2m
Copy link
Member

gr2m commented Nov 7, 2021

@paulmelnikow would you like to finish up your work here? We can open it up to other contributors otherwise

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

root url parsing needs to be more robust/lenient
3 participants