-
Notifications
You must be signed in to change notification settings - Fork 330
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
Use web_sys::Url for IDN conversion on wasm32-unknown-unknown #887
Open
micolous
wants to merge
10
commits into
servo:main
Choose a base branch
from
micolous:wasm
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Nov 28, 2023
While this is based on #886, I've marked it as "ready for review", as this isn't really a work in progress – it's just blocked. |
* remove `Url::socket_addrs` on wasm32-unknown-unknown (it won't work, those platform API calls are not supported) * disable unit tests which won't work on wasm32-unknown-unknown * run tests in `wasm_bindgen_test` on wasm32-unknown-unknown * remove `panic::catch_unwind` from wpt tests, as that conflicts with wasm-bindgen's panic handler * run the tests in CI
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #887 +/- ##
=======================================
Coverage ? 81.88%
=======================================
Files ? 20
Lines ? 3545
Branches ? 0
=======================================
Hits ? 2903
Misses ? 642
Partials ? 0 ☔ View full report in Codecov by Sentry. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request is based on #886. Merging this PR will also merge #886.
This removes the dependency on
idna
onwasm32-unknown-unknown
, replacing it with a small shim which calls out toweb_sys::Url
.With a small
yew
demo app (which callsUrl::parse()
), this reduces binary size by 228 KiB on a release build and by 425 KiB on a debug build. These savings should go a lot of the way towards addressing #557, while retaining IDN support.API changes
The deprecations could be split out from this, I've included them as related work.
Deprecate
quirks::domain_to_unicode()
: Remove URL.domainToASCII and URL.domainToUnicode whatwg/url#63Removed on
wasm32-unknown-unknown
because browsers never implemented it.Deprecate
Origin::unicode_serialization()
: Mark Origin::unicode_serialization as deprecated? #881Removed on
wasm32-unknown-unknown
because it requiresdomain_to_unicode
.Deprecate
quirks::domain_to_ascii()
: Remove URL.domainToASCII and URL.domainToUnicode whatwg/url#63Kept on
wasm32-unknown-unknown
, because its implementation does not depend on browser functionality.Alternatives considered
Downstream libraries could adopt
web_sys::Url
onwasm32-unknown-unknown
.This is API-breaking, and complicated to handle, for reasons I've covered in WASM file size #557 and Use web_sys::Url instead of url::Url on wasm32 targets seanmonstar/reqwest#2050.
There are similar pains discussed in Use url::Url instead of http::Uri sagebind/isahc#382 for a
http::Uri
tourl::Url
migration.Shimming out every single piece of functionality to JavaScript's
Url
type (viaweb_sys::Url
) onwasm32-unknown-unknown
.This would probably make things smaller again, but there are enough subtle API differences to make this a problem. For example,
url::Url::host()
returns aurl::Host
, butweb_sys::Url::hostname()
returns aString
.Shimming in
idna
.Not all the functionality required is supported in web APIs (
domain_to_unicode
), and so it needs changes toUrl
anyway.This also allows one to continue using the
idna
crate directly onwasm32-unknown-unknown
with its normalisation database.Known issues
There are a few extra test failures (see
expected_failures_{chromium,firefox,safari}.txt
) – I'm not certain which of these are browser bugs orurl::Host
bugs.In any case, I don't think I can fix them, and I don't know how much one should care.
I haven't tried this on
redox
orwasi
targets – they don't appear to be covered by CI, and so I've left them as-is.It may be possible to get similar binary size savings on those targets, but I'm keeping them out of scope for now.