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

url: call onParsePortComplete on null port from onParseHostComplete #20476

Closed
wants to merge 1 commit into from

Conversation

AyushG3112
Copy link
Contributor

@AyushG3112 AyushG3112 commented May 2, 2018

Call onParsePortComplete on null port from onParseHostComplete to
set url’s port to null, if port is url’s scheme’s default port,
and to port otherwise, as specified in the spec.

Also, trim all trailing : from host in the setter to fix
regressions caused by the change.

Refs: https://url.spec.whatwg.org/#port-state
Fixes: #20465

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

Call `onParsePortComplete` on null port from `onParseHostComplete` to
set url’s port to null, if port is url’s scheme’s default port,
and to port otherwise, as specified in the spec.

Also, trim all trailing `:` from `host` in the setter to fix
regressions caused by the change.

Refs: https://url.spec.whatwg.org/#port-state
Fixes: nodejs#20465
@nodejs-github-bot nodejs-github-bot added dont-land-on-v4.x whatwg-url Issues and PRs related to the WHATWG URL implementation. labels May 2, 2018
@AyushG3112
Copy link
Contributor Author

AyushG3112 commented May 2, 2018

@ nodejs/testing Could anyone please point me to where I should add the tests? The fixture used in the existing test is the one provided by W3C and I'm not sure if I should modify that.

@AyushG3112
Copy link
Contributor Author

Closing in favor of an upcoming better PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

assigning a hostname with port 80 to URL.host will not override the existing port
2 participants