FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

url: remove #context from URLSearchParams · nodejs/node@925a464 · GitHub

/ node Public

Commit 925a464

Browse files
MattIPv4
authored
url: remove #context from URLSearchParams
PR-URL: #51520 Fixes: #51518 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
1 parent a3abc42 commit 925a464

5 files changed

Lines changed: 176 additions & 19 deletions

File tree

‎benchmark/url/url-searchparams-append.js‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
'use strict';
2+
const common = require('../common.js');
3+
4+
const bench = common.createBenchmark(main, {
5+
type: ['URL', 'URLSearchParams'],
6+
n: [1e3, 1e6],
7+
});
8+
9+
function main({ type, n }) {
10+
const params = type === 'URL' ?
11+
new URL('https://nodejs.org').searchParams :
12+
new URLSearchParams();
13+
14+
bench.start();
15+
for (let i = 0; i < n; i++) {
16+
params.append('test', i);
17+
}
18+
bench.end(n);
19+
}

‎benchmark/url/url-searchparams-update.js‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
'use strict';
2+
const common = require('../common.js');
3+
const assert = require('assert');
4+
5+
const bench = common.createBenchmark(main, {
6+
searchParams: ['true', 'false'],
7+
property: ['pathname', 'search', 'hash'],
8+
n: [1e6],
9+
});
10+
11+
function getMethod(url, property) {
12+
if (property === 'pathname') return (x) => url.pathname = `/${x}`;
13+
if (property === 'search') return (x) => url.search = `?${x}`;
14+
if (property === 'hash') return (x) => url.hash = `#${x}`;
15+
throw new Error(`Unsupported property "${property}"`);
16+
}
17+
18+
function main({ searchParams, property, n }) {
19+
const url = new URL('https://nodejs.org');
20+
if (searchParams === 'true') assert(url.searchParams);
21+
22+
const method = getMethod(url, property);
23+
24+
bench.start();
25+
for (let i = 0; i < n; i++) {
26+
method(i);
27+
}
28+
bench.end(n);
29+
}

‎lib/internal/url.js‎

Lines changed: 77 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -206,6 +206,7 @@ class URLContext {
206206
}
207207
}
208208

209+
let setURLSearchParamsModified;
209210
let setURLSearchParamsContext;
210211
let getURLSearchParamsList;
211212
let setURLSearchParams;
@@ -475,8 +476,9 @@ class URLSearchParams {
475476
name = StringPrototypeToWellFormed(`${name}`);
476477
value = StringPrototypeToWellFormed(`${value}`);
477478
ArrayPrototypePush(this.#searchParams, name, value);
479+
478480
if (this.#context) {
479-
this.#context.search = this.toString();
481+
setURLSearchParamsModified(this.#context);
480482
}
481483
}
482484

@@ -509,8 +511,9 @@ class URLSearchParams {
509511
}
510512
}
511513
}
514+
512515
if (this.#context) {
513-
this.#context.search = this.toString();
516+
setURLSearchParamsModified(this.#context);
514517
}
515518
}
516519

@@ -615,7 +618,7 @@ class URLSearchParams {
615618
}
616619

617620
if (this.#context) {
618-
this.#context.search = this.toString();
621+
setURLSearchParamsModified(this.#context);
619622
}
620623
}
621624

@@ -664,7 +667,7 @@ class URLSearchParams {
664667
}
665668

666669
if (this.#context) {
667-
this.#context.search = this.toString();
670+
setURLSearchParamsModified(this.#context);
668671
}
669672
}
670673

@@ -769,6 +772,20 @@ function isURL(self) {
769772
class URL {
770773
#context = new URLContext();
771774
#searchParams;
775+
#searchParamsModified;
776+
777+
static {
778+
setURLSearchParamsModified = (obj) => {
779+
// When URLSearchParams changes, we lazily update URL on the next read/write for performance.
780+
obj.#searchParamsModified = true;
781+
782+
// If URL has an existing search, remove it without cascading back to URLSearchParams.
783+
// Do this to avoid any internal confusion about whether URLSearchParams or URL is up-to-date.
784+
if (obj.#context.hasSearch) {
785+
obj.#updateContext(bindingUrl.update(obj.#context.href, updateActions.kSearch, ''));
786+
}
787+
};
788+
}
772789

773790
constructor(input, base = undefined) {
774791
markTransferMode(this, false, false);
@@ -814,7 +831,37 @@ class URL {
814831
return `${constructor.name} ${inspect(obj, opts)}`;
815832
}
816833

817-
#updateContext(href) {
834+
#getSearchFromContext() {
835+
if (!this.#context.hasSearch) return '';
836+
let endsAt = this.#context.href.length;
837+
if (this.#context.hasHash) endsAt = this.#context.hash_start;
838+
if (endsAt - this.#context.search_start <= 1) return '';
839+
return StringPrototypeSlice(this.#context.href, this.#context.search_start, endsAt);
840+
}
841+
842+
#getSearchFromParams() {
843+
if (!this.#searchParams?.size) return '';
844+
return `?${this.#searchParams}`;
845+
}
846+
847+
#ensureSearchParamsUpdated() {
848+
// URL is updated lazily to greatly improve performance when URLSearchParams is updated repeatedly.
849+
// If URLSearchParams has been modified, reflect that back into URL, without cascading back.
850+
if (this.#searchParamsModified) {
851+
this.#searchParamsModified = false;
852+
this.#updateContext(bindingUrl.update(this.#context.href, updateActions.kSearch, this.#getSearchFromParams()));
853+
}
854+
}
855+
856+
/**
857+
* Update the internal context state for URL.
858+
* @param {string} href New href string from `bindingUrl.update`.
859+
* @param {boolean} [shouldUpdateSearchParams] If the update has potential to update search params (href/search).
860+
*/
861+
#updateContext(href, shouldUpdateSearchParams = false) {
862+
const previousSearch = shouldUpdateSearchParams && this.#searchParams &&
863+
(this.#searchParamsModified ? this.#getSearchFromParams() : this.#getSearchFromContext());
864+
818865
this.#context.href = href;
819866

820867
const {
@@ -840,27 +887,39 @@ class URL {
840887
this.#context.scheme_type = scheme_type;
841888

842889
if (this.#searchParams) {
843-
if (this.#context.hasSearch) {
844-
setURLSearchParams(this.#searchParams, this.search);
845-
} else {
846-
setURLSearchParams(this.#searchParams, undefined);
890+
// If the search string has updated, URL becomes the source of truth, and we update URLSearchParams.
891+
// Only do this when we're expecting it to have changed, otherwise a change to hash etc.
892+
// would incorrectly compare the URLSearchParams state to the empty URL search state.
893+
if (shouldUpdateSearchParams) {
894+
const currentSearch = this.#getSearchFromContext();
895+
if (previousSearch !== currentSearch) {
896+
setURLSearchParams(this.#searchParams, currentSearch);
897+
this.#searchParamsModified = false;
898+
}
847899
}
900+
901+
// If we have a URLSearchParams, ensure that URL is up-to-date with any modification to it.
902+
this.#ensureSearchParamsUpdated();
848903
}
849904
}
850905

851906
toString() {
907+
// Updates to URLSearchParams are lazily propagated to URL, so we need to check we're in sync.
908+
this.#ensureSearchParamsUpdated();
852909
return this.#context.href;
853910
}
854911

855912
get href() {
913+
// Updates to URLSearchParams are lazily propagated to URL, so we need to check we're in sync.
914+
this.#ensureSearchParamsUpdated();
856915
return this.#context.href;
857916
}
858917

859918
set href(value) {
860919
value = `${value}`;
861920
const href = bindingUrl.update(this.#context.href, updateActions.kHref, value);
862921
if (!href) { throw new ERR_INVALID_URL(value); }
863-
this.#updateContext(href);
922+
this.#updateContext(href, true);
864923
}
865924

866925
// readonly
@@ -1002,26 +1061,25 @@ class URL {
10021061
}
10031062

10041063
get search() {
1005-
if (!this.#context.hasSearch) { return ''; }
1006-
let endsAt = this.#context.href.length;
1007-
if (this.#context.hasHash) { endsAt = this.#context.hash_start; }
1008-
if (endsAt - this.#context.search_start <= 1) { return ''; }
1009-
return StringPrototypeSlice(this.#context.href, this.#context.search_start, endsAt);
1064+
// Updates to URLSearchParams are lazily propagated to URL, so we need to check we're in sync.
1065+
this.#ensureSearchParamsUpdated();
1066+
return this.#getSearchFromContext();
10101067
}
10111068

10121069
set search(value) {
10131070
const href = bindingUrl.update(this.#context.href, updateActions.kSearch, StringPrototypeToWellFormed(`${value}`));
10141071
if (href) {
1015-
this.#updateContext(href);
1072+
this.#updateContext(href, true);
10161073
}
10171074
}
10181075

10191076
// readonly
10201077
get searchParams() {
10211078
// Create URLSearchParams on demand to greatly improve the URL performance.
10221079
if (this.#searchParams == null) {
1023-
this.#searchParams = new URLSearchParams(this.search);
1080+
this.#searchParams = new URLSearchParams(this.#getSearchFromContext());
10241081
setURLSearchParamsContext(this.#searchParams, this);
1082+
this.#searchParamsModified = false;
10251083
}
10261084
return this.#searchParams;
10271085
}
@@ -1041,6 +1099,8 @@ class URL {
10411099
}
10421100

10431101
toJSON() {
1102+
// Updates to URLSearchParams are lazily propagated to URL, so we need to check we're in sync.
1103+
this.#ensureSearchParamsUpdated();
10441104
return this.#context.href;
10451105
}
10461106

‎test/parallel/test-whatwg-url-custom-searchparams.js‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,42 @@ assert.strictEqual(sp.toString(), serialized);
4343

4444
assert.strictEqual(m.search, `?${serialized}`);
4545

46+
sp.delete('a');
47+
values.forEach((i) => sp.append('a', i));
48+
assert.strictEqual(m.href, `http://example.org/?${serialized}`);
49+
50+
sp.delete('a');
51+
values.forEach((i) => sp.append('a', i));
52+
assert.strictEqual(m.toString(), `http://example.org/?${serialized}`);
53+
54+
sp.delete('a');
55+
values.forEach((i) => sp.append('a', i));
56+
assert.strictEqual(m.toJSON(), `http://example.org/?${serialized}`);
57+
58+
sp.delete('a');
59+
values.forEach((i) => sp.append('a', i));
60+
m.href = 'http://example.org';
61+
assert.strictEqual(m.href, 'http://example.org/');
62+
assert.strictEqual(sp.size, 0);
63+
64+
sp.delete('a');
65+
values.forEach((i) => sp.append('a', i));
66+
m.search = '';
67+
assert.strictEqual(m.href, 'http://example.org/');
68+
assert.strictEqual(sp.size, 0);
69+
70+
sp.delete('a');
71+
values.forEach((i) => sp.append('a', i));
72+
m.pathname = '/test';
73+
assert.strictEqual(m.href, `http://example.org/test?${serialized}`);
74+
m.pathname = '';
75+
76+
sp.delete('a');
77+
values.forEach((i) => sp.append('a', i));
78+
m.hash = '#test';
79+
assert.strictEqual(m.href, `http://example.org/?${serialized}#test`);
80+
m.hash = '';
81+
4682
assert.strictEqual(sp[Symbol.iterator], sp.entries);
4783

4884
let key, val;

‎test/parallel/test-whatwg-url-invalidthis.js‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,20 +11,33 @@ const assert = require('assert');
1111
].forEach((i) => {
1212
assert.throws(() => Reflect.apply(URL.prototype[i], [], {}), {
1313
name: 'TypeError',
14-
message: /Cannot read private member/,
14+
message: /Receiver must be an instance of class/,
1515
});
1616
});
1717

1818
[
1919
'href',
20+
'search',
21+
].forEach((i) => {
22+
assert.throws(() => Reflect.get(URL.prototype, i, {}), {
23+
name: 'TypeError',
24+
message: /Receiver must be an instance of class/,
25+
});
26+
27+
assert.throws(() => Reflect.set(URL.prototype, i, null, {}), {
28+
name: 'TypeError',
29+
message: /Cannot read private member/,
30+
});
31+
});
32+
33+
[
2034
'protocol',
2135
'username',
2236
'password',
2337
'host',
2438
'hostname',
2539
'port',
2640
'pathname',
27-
'search',
2841
'hash',
2942
].forEach((i) => {
3043
assert.throws(() => Reflect.get(URL.prototype, i, {}), {

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL