Skip to content

canonicalize_port should use url::quirks::set_port#59

Merged
lucacasonato merged 3 commits into
denoland:mainfrom
valenting:main
Jun 27, 2025
Merged

canonicalize_port should use url::quirks::set_port#59
lucacasonato merged 3 commits into
denoland:mainfrom
valenting:main

Conversation

@valenting
Copy link
Copy Markdown
Contributor

Fixes #53

https://urlpattern.spec.whatwg.org/commit-snapshots/696b4029d52e5854044bac6b72cdb198cb962ed0/#canonicalize-a-port

Let parseResult be the result of running basic URL parser given portValue with dummyURL as url and port state as state override.

To do this we must use url::quirks::set_port instead of the url::set_port method which takes a u16.
Additionally this PR syncs the test data, except for a few which don't deserialize since they contained invalid UTF8.

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Jun 26, 2025

CLA assistant check
All committers have signed the CLA.

@valenting
Copy link
Copy Markdown
Contributor Author

@lucacasonato the CI is hanging for some reason. Could you retrigger it for me?

Comment thread src/lib.rs Outdated
}

let expected = expected.unwrap();
let expected = expected.trim();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Wasn't. Thanks for pointing it out.

Copy link
Copy Markdown
Contributor

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

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

LGTM

@lucacasonato lucacasonato merged commit e2c6c2c into denoland:main Jun 27, 2025
2 checks passed
@valenting
Copy link
Copy Markdown
Contributor Author

@lucacasonato FYI, this crate is now backing our urlpattern implementation in Firefox 142
1948330 - ms1 Add denoland's urlpattern to mozilla-central
Thanks for the great work!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

canonicalizePort does not follow spec.

3 participants