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

perf(ext/fetch): skip USVString webidl conv on string constructor #12168

Merged
merged 5 commits into from
Sep 23, 2021

Conversation

AaronO
Copy link
Contributor

@AaronO AaronO commented Sep 21, 2021

Since URL parsing will effectively normalize it, this is essentially another redundant webidl conversion

Assumes #12167

Since URL parsing will effectively normalize it, this is essentially another redundant webidl conversion

Assumes denoland#12167
@andreubotella
Copy link
Contributor

andreubotella commented Sep 21, 2021

The conversion to RequestInfo first checks if the value is an instanceof Request, and if it isn't, it turns it into a USVString – not the other way around.

See https://heycam.github.io/webidl/#es-union

@AaronO
Copy link
Contributor Author

AaronO commented Sep 21, 2021

The conversion to RequestInfo first checks if the value is an instanceof Request, and if it isn't, it turns it into a USVString – not the other way around.

See https://heycam.github.io/webidl/#es-union

Yeah the alternative patch was to use DOMString instead of USVString since op_url_parse (or any op for that matter) will implicitly do DOMString -> USVString, I'll submit that

@AaronO
Copy link
Contributor Author

AaronO commented Sep 21, 2021

@andreubotella Please take another look

To disambiguate and hint that it normalizes to DOMString instead of USVString since DOMString => USVString is handled by `op_url_parse` when calling `new URL(...)`
Copy link
Member

@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

@AaronO AaronO merged commit e0c858f into denoland:main Sep 23, 2021
@AaronO AaronO deleted the perf/new-request-webidl branch September 23, 2021 09:41
caspervonb pushed a commit to caspervonb/deno that referenced this pull request Sep 26, 2021
…noland#12168)

* perf(ext/fetch): skip USVString webidl conv on string constructor
* Rename webidl convert to RequestInfo_DOMString

To disambiguate and hint that it normalizes to DOMString instead of USVString since DOMString => USVString is handled by `op_url_parse` when calling `new URL(...)`
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.

3 participants