url: fix definitions of URL/SearchParams methods and accessors#36799
Merged
aduh95 merged 2 commits intonodejs:masterfrom Feb 13, 2021
Merged
url: fix definitions of URL/SearchParams methods and accessors#36799aduh95 merged 2 commits intonodejs:masterfrom
URL/SearchParams methods and accessors#36799aduh95 merged 2 commits intonodejs:masterfrom
Conversation
Contributor
|
Would you kindly split this into several commits to help with code review please? GitHub UI isn't really helping to follow the moving parts here 😅 |
Flarna
reviewed
Jan 5, 2021
Flarna
reviewed
Jan 5, 2021
ExE-Boss
force-pushed
the
lib/url/fix-method-accessor-definitions
branch
2 times, most recently
from
076b097 to
335ae15
Compare
Collaborator
Collaborator
Collaborator
Collaborator
Contributor
|
@ExE-Boss This needs a rebase. If you have the time to split it into several commits (E.G.: one for |
Contributor
Author
|
@aduh95 I have split this into a move (0a49b9e) and edit (609a6eb?w=1) commit. |
ExE-Boss
force-pushed
the
lib/url/fix-method-accessor-definitions
branch
from
335ae15 to
23386ce
Compare
Contributor
|
Nit: upstreaming the tests to the wpt repo, so other implementers can use them, would be ideal. |
Contributor
Author
Flarna
added
whatwg-url
Contributor
Contributor
|
@ExE-Boss this needs a rebase. |
ExE-Boss
force-pushed
the
lib/url/fix-method-accessor-definitions
branch
from
23386ce to
609a6eb
Compare
Collaborator
Contributor
|
There are some major perf improvements and also a few regressions for the legacy API for some reason: WHATWG results look OK, I'm going to spawn another benchmark CI to see if the perf change on legacy API were just a flakiness. Full benchmark results |
Contributor
|
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/941/ EDIT: it got similar results 🤷♂️ Details |
Collaborator
aduh95
approved these changes
Feb 9, 2021
aduh95
added
the
author ready
Contributor
Author
I’m guessing that’s because V8 is able to better optimise classes when the methods are defined in the class declaration, rather than using |
Member
|
cc @nodejs/url |
Contributor
OK but do you understand how does this affect the legacy API? |
TimothyGu
approved these changes
Feb 10, 2021
PR-URL: nodejs#36799 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
This fixes getter and setter names for the WHATWG URL classes, and fixes a few other inconsistencies with browsers implementations. Co-authored-by: Gerhard Stöbich <[email protected]> PR-URL: nodejs#36799 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
aduh95
force-pushed
the
lib/url/fix-method-accessor-definitions
branch
from
609a6eb to
0dfc0f5
Compare
Contributor
|
Landed in 7d5806e...0dfc0f5 |
danielleadams
pushed a commit
that referenced
this pull request
Feb 16, 2021
PR-URL: #36799 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
danielleadams
pushed a commit
that referenced
this pull request
Feb 16, 2021
This fixes getter and setter names for the WHATWG URL classes, and fixes a few other inconsistencies with browsers implementations. Co-authored-by: Gerhard Stöbich <[email protected]> PR-URL: #36799 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
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 ensures that:
and that:
The changes to
URLSearchParamswere done to fix the[[HomeObject]]internal slot so that it points to theURLSearchParams.prototypeobject, rather than the object that’s passed todefineIDLClass.Best reviewed with the “Hide whitespace changes” flag enabled: #36799?w=1