-
-
Notifications
You must be signed in to change notification settings - Fork 35.4k
url: don't update URL immediately on update to URLSearchParams #51520
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
Changes from 13 commits
2b2d11b
129a1a9
c2e8526
039380e
e015de2
2487726
b0270bc
d9c5381
a05675c
310d275
b118a90
916374e
319eaa0
0c44fb6
065c45c
0d0fef8
e9c4da2
04f53a7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| 'use strict'; | ||
| const common = require('../common.js'); | ||
|
|
||
| const bench = common.createBenchmark(main, { | ||
| type: ['URL', 'URLSearchParams'], | ||
| n: [1e3, 1e6], | ||
| }); | ||
|
|
||
| function main({ type, n }) { | ||
| const params = type === 'URL' ? | ||
| new URL('https://nodejs.org').searchParams : | ||
| new URLSearchParams(); | ||
|
|
||
| bench.start(); | ||
| for (let i = 0; i < n; i++) { | ||
| params.append('test', i); | ||
| } | ||
| bench.end(n); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| 'use strict'; | ||
| const common = require('../common.js'); | ||
|
|
||
| const bench = common.createBenchmark(main, { | ||
| searchParams: ['true', 'false'], | ||
| property: ['pathname', 'search'], | ||
| n: [1e6], | ||
| }); | ||
|
|
||
| function main({ searchParams, property, n }) { | ||
| const url = new URL('https://nodejs.org'); | ||
| if (searchParams === 'true') url.searchParams; // eslint-disable-line no-unused-expressions | ||
|
MattIPv4 marked this conversation as resolved.
Outdated
|
||
|
|
||
| const method = property === 'pathname' ? | ||
| (x) => url.pathname = `/${x}` : | ||
| (x) => url.search = `?${x}`; | ||
|
|
||
| bench.start(); | ||
| for (let i = 0; i < n; i++) { | ||
| method(i); | ||
| } | ||
| bench.end(n); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -206,6 +206,7 @@ class URLContext { | |
| } | ||
| } | ||
|
|
||
| let setURLSearchParamsModified; | ||
| let setURLSearchParamsContext; | ||
| let getURLSearchParamsList; | ||
| let setURLSearchParams; | ||
|
|
@@ -475,8 +476,9 @@ class URLSearchParams { | |
| name = StringPrototypeToWellFormed(`${name}`); | ||
| value = StringPrototypeToWellFormed(`${value}`); | ||
| ArrayPrototypePush(this.#searchParams, name, value); | ||
|
|
||
| if (this.#context) { | ||
| this.#context.search = this.toString(); | ||
| setURLSearchParamsModified(this.#context); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -509,8 +511,9 @@ class URLSearchParams { | |
| } | ||
| } | ||
| } | ||
|
|
||
| if (this.#context) { | ||
| this.#context.search = this.toString(); | ||
| setURLSearchParamsModified(this.#context); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -615,7 +618,7 @@ class URLSearchParams { | |
| } | ||
|
|
||
| if (this.#context) { | ||
| this.#context.search = this.toString(); | ||
| setURLSearchParamsModified(this.#context); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -664,7 +667,7 @@ class URLSearchParams { | |
| } | ||
|
|
||
| if (this.#context) { | ||
| this.#context.search = this.toString(); | ||
| setURLSearchParamsModified(this.#context); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -769,6 +772,20 @@ function isURL(self) { | |
| class URL { | ||
| #context = new URLContext(); | ||
| #searchParams; | ||
| #searchParamsModified; | ||
|
|
||
| static { | ||
| setURLSearchParamsModified = (obj) => { | ||
| // When URLSearchParams changes, we lazily update URL on the next read/write for performance. | ||
| obj.#searchParamsModified = true; | ||
|
|
||
| // If URL has an existing search, remove it without cascading back to URLSearchParams. | ||
| // Do this to avoid any internal confusion about whether URLSearchParams or URL is up-to-date. | ||
| if (obj.#context.hasSearch) { | ||
| obj.#updateContext(bindingUrl.update(obj.#context.href, updateActions.kSearch, ''), false); | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| constructor(input, base = undefined) { | ||
| markTransferMode(this, false, false); | ||
|
|
@@ -814,7 +831,34 @@ class URL { | |
| return `${constructor.name} ${inspect(obj, opts)}`; | ||
| } | ||
|
|
||
| #updateContext(href) { | ||
| #getSearchFromContext() { | ||
| if (!this.#context.hasSearch) return ''; | ||
| let endsAt = this.#context.href.length; | ||
| if (this.#context.hasHash) endsAt = this.#context.hash_start; | ||
| if (endsAt - this.#context.search_start <= 1) return ''; | ||
| return StringPrototypeSlice(this.#context.href, this.#context.search_start, endsAt); | ||
| } | ||
|
|
||
| #getSearchFromParams() { | ||
| if (!this.#searchParams) return ''; | ||
| if (!this.#searchParams.size) return ''; | ||
|
MattIPv4 marked this conversation as resolved.
Outdated
|
||
| return `?${this.#searchParams.toString()}`; | ||
|
MattIPv4 marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| #checkSearchParams() { | ||
|
MattIPv4 marked this conversation as resolved.
Outdated
|
||
| // If URLSearchParams has been modified, reflect that back into URL, and do not cascade back. | ||
| // This is done lazily to greatly improve performance when URLSearchParams is updated repeatedly. | ||
| if (this.#searchParamsModified) { | ||
| const href = bindingUrl.update(this.#context.href, updateActions.kSearch, this.#getSearchFromParams()); | ||
| this.#updateContext(href, false); | ||
| this.#searchParamsModified = false; | ||
| } | ||
| } | ||
|
|
||
| #updateContext(href, updateSearchParams = !!this.#searchParams) { | ||
|
MattIPv4 marked this conversation as resolved.
Outdated
|
||
| const previousSearch = updateSearchParams && (this.#searchParamsModified ? | ||
| this.#getSearchFromParams() : this.#getSearchFromContext()); | ||
|
|
||
| this.#context.href = href; | ||
|
|
||
| const { | ||
|
|
@@ -839,20 +883,26 @@ class URL { | |
| this.#context.hash_start = hash_start; | ||
| this.#context.scheme_type = scheme_type; | ||
|
|
||
| if (this.#searchParams) { | ||
| if (this.#context.hasSearch) { | ||
| setURLSearchParams(this.#searchParams, this.search); | ||
| // If URLSearchParams exists, and the search has changed, update it. | ||
| // Otherwise, check if URLSearchParams has been modified. | ||
| const currentSearch = updateSearchParams && this.#getSearchFromContext(); | ||
| if (updateSearchParams) { | ||
|
MattIPv4 marked this conversation as resolved.
Outdated
MattIPv4 marked this conversation as resolved.
Outdated
|
||
| if (previousSearch !== currentSearch) { | ||
| setURLSearchParams(this.#searchParams, currentSearch); | ||
| this.#searchParamsModified = false; | ||
| } else { | ||
| setURLSearchParams(this.#searchParams, undefined); | ||
| this.#checkSearchParams(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| toString() { | ||
| this.#checkSearchParams(); | ||
| return this.#context.href; | ||
| } | ||
|
|
||
| get href() { | ||
| this.#checkSearchParams(); | ||
| return this.#context.href; | ||
| } | ||
|
|
||
|
|
@@ -1002,11 +1052,8 @@ class URL { | |
| } | ||
|
|
||
| get search() { | ||
| if (!this.#context.hasSearch) { return ''; } | ||
| let endsAt = this.#context.href.length; | ||
| if (this.#context.hasHash) { endsAt = this.#context.hash_start; } | ||
| if (endsAt - this.#context.search_start <= 1) { return ''; } | ||
| return StringPrototypeSlice(this.#context.href, this.#context.search_start, endsAt); | ||
| this.#checkSearchParams(); | ||
| return this.#getSearchFromContext(); | ||
| } | ||
|
|
||
| set search(value) { | ||
|
|
@@ -1020,8 +1067,9 @@ class URL { | |
| get searchParams() { | ||
| // Create URLSearchParams on demand to greatly improve the URL performance. | ||
| if (this.#searchParams == null) { | ||
| this.#searchParams = new URLSearchParams(this.search); | ||
| this.#searchParams = new URLSearchParams(this.#getSearchFromContext()); | ||
| setURLSearchParamsContext(this.#searchParams, this); | ||
| this.#searchParamsModified = false; | ||
| } | ||
| return this.#searchParams; | ||
| } | ||
|
|
@@ -1041,6 +1089,7 @@ class URL { | |
| } | ||
|
|
||
| toJSON() { | ||
| this.#checkSearchParams(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this function can just be
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Am I fine to also swap or should
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. toJSON and toString should call this.href, since technically they are
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Switching these over to
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because you need private property access to check if the current
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I do this though, the WPT tests pass, but
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thinking this through more, is this throwing a different exception now because it is trying to access a private method first ( |
||
| return this.#context.href; | ||
| } | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.