url: don't update URL immediately on update to URLSearchParams

PR-URL: #51520Fixes: #51518 Backport-PR-URL: #51559 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

Author: MattIPv4

benchmark/url/url-searchparams-append.js

@@ -0,0 +1,19 @@
+'use strict';
+constcommon=require('../common.js');
+
+constbench=common.createBenchmark(main,{
+type: ['URL','URLSearchParams'],
+n: [1e3,1e6],
+});
+
+functionmain({ type, n }){
+constparams=type==='URL' ?
+newURL('https://nodejs.org').searchParams :
+newURLSearchParams();
+
+bench.start();
+for(leti=0;i<n;i++){
+params.append('test',i);
+}
+bench.end(n);
+}

benchmark/url/url-searchparams-update.js

@@ -0,0 +1,29 @@
+'use strict';
+constcommon=require('../common.js');
+constassert=require('assert');
+
+constbench=common.createBenchmark(main,{
+searchParams: ['true','false'],
+property: ['pathname','search','hash'],
+n: [1e6],
+});
+
+functiongetMethod(url,property){
+if(property==='pathname')return(x)=>url.pathname=`/${x}`;
+if(property==='search')return(x)=>url.search=`?${x}`;
+if(property==='hash')return(x)=>url.hash=`#${x}`;
+thrownewError(`Unsupported property "${property}"`);
+}
+
+functionmain({ searchParams, property, n }){
+consturl=newURL('https://nodejs.org');
+if(searchParams==='true')assert(url.searchParams);
+
+constmethod=getMethod(url,property);
+
+bench.start();
+for(leti=0;i<n;i++){
+method(i);
+}
+bench.end(n);
+}

lib/internal/url.js

@@ -202,6 +202,7 @@ class URLContext{
 }
 }
 
+letsetURLSearchParamsModified;
 letsetURLSearchParamsContext;
 letgetURLSearchParamsList;
 letsetURLSearchParams;
@@ -469,8 +470,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);
 }
 }
 
@@ -503,8 +505,9 @@ class URLSearchParams{
 }
 }
 }
+
 if(this.#context){
-this.#context.search=this.toString();
+setURLSearchParamsModified(this.#context);
 }
 }
 
@@ -609,7 +612,7 @@ class URLSearchParams{
 }
 
 if(this.#context){
-this.#context.search=this.toString();
+setURLSearchParamsModified(this.#context);
 }
 }
 
@@ -658,7 +661,7 @@ class URLSearchParams{
 }
 
 if(this.#context){
-this.#context.search=this.toString();
+setURLSearchParamsModified(this.#context);
 }
 }
 
@@ -763,6 +766,20 @@ function isURL(self){
 classURL{
  #context =newURLContext();
  #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,''));
+}
+};
+}
 
 constructor(input,base=undefined){
 if(arguments.length===0){
@@ -806,7 +823,37 @@ class URL{
 return`${constructor.name}${inspect(obj,opts)}`;
 }
 
- #updateContext(href){
+ #getSearchFromContext(){
+if(!this.#context.hasSearch)return'';
+letendsAt=this.#context.href.length;
+if(this.#context.hasHash)endsAt=this.#context.hash_start;
+if(endsAt-this.#context.search_start<=1)return'';
+returnStringPrototypeSlice(this.#context.href,this.#context.search_start,endsAt);
+}
+
+ #getSearchFromParams(){
+if(!this.#searchParams?.size)return'';
+return`?${this.#searchParams}`;
+}
+
+ #ensureSearchParamsUpdated(){
+// URL is updated lazily to greatly improve performance when URLSearchParams is updated repeatedly.
+// If URLSearchParams has been modified, reflect that back into URL, without cascading back.
+if(this.#searchParamsModified){
+this.#searchParamsModified =false;
+this.#updateContext(bindingUrl.update(this.#context.href,updateActions.kSearch,this.#getSearchFromParams()));
+}
+}
+
+/**
+ * Update the internal context state for URL.
+ * @param{string} href New href string from `bindingUrl.update`.
+ * @param{boolean} [shouldUpdateSearchParams] If the update has potential to update search params (href/search).
+ */
+ #updateContext(href,shouldUpdateSearchParams=false){
+constpreviousSearch=shouldUpdateSearchParams&&this.#searchParams &&
+(this.#searchParamsModified ? this.#getSearchFromParams() : this.#getSearchFromContext());
+
 this.#context.href=href;
 
 const{
@@ -832,27 +879,39 @@ class URL{
 this.#context.scheme_type=scheme_type;
 
 if(this.#searchParams){
-if(this.#context.hasSearch){
-setURLSearchParams(this.#searchParams,this.search);
-}else{
-setURLSearchParams(this.#searchParams,undefined);
+// If the search string has updated, URL becomes the source of truth, and we update URLSearchParams.
+// Only do this when we're expecting it to have changed, otherwise a change to hash etc.
+// would incorrectly compare the URLSearchParams state to the empty URL search state.
+if(shouldUpdateSearchParams){
+constcurrentSearch=this.#getSearchFromContext();
+if(previousSearch!==currentSearch){
+setURLSearchParams(this.#searchParams,currentSearch);
+this.#searchParamsModified =false;
+}
 }
+
+// If we have a URLSearchParams, ensure that URL is up-to-date with any modification to it.
+this.#ensureSearchParamsUpdated();
 }
 }
 
 toString(){
+// Updates to URLSearchParams are lazily propagated to URL, so we need to check we're in sync.
+this.#ensureSearchParamsUpdated();
 returnthis.#context.href;
 }
 
 gethref(){
+// Updates to URLSearchParams are lazily propagated to URL, so we need to check we're in sync.
+this.#ensureSearchParamsUpdated();
 returnthis.#context.href;
 }
 
 sethref(value){
 value=`${value}`;
 consthref=bindingUrl.update(this.#context.href,updateActions.kHref,value);
 if(!href){thrownewERR_INVALID_URL(value);}
-this.#updateContext(href);
+this.#updateContext(href,true);
 }
 
 // readonly
@@ -994,26 +1053,25 @@ class URL{
 }
 
 getsearch(){
-if(!this.#context.hasSearch){return'';}
-letendsAt=this.#context.href.length;
-if(this.#context.hasHash){endsAt=this.#context.hash_start;}
-if(endsAt-this.#context.search_start<=1){return'';}
-returnStringPrototypeSlice(this.#context.href,this.#context.search_start,endsAt);
+// Updates to URLSearchParams are lazily propagated to URL, so we need to check we're in sync.
+this.#ensureSearchParamsUpdated();
+returnthis.#getSearchFromContext();
 }
 
 setsearch(value){
 consthref=bindingUrl.update(this.#context.href,updateActions.kSearch,StringPrototypeToWellFormed(`${value}`));
 if(href){
-this.#updateContext(href);
+this.#updateContext(href,true);
 }
 }
 
 // readonly
 getsearchParams(){
 // Create URLSearchParams on demand to greatly improve the URL performance.
 if(this.#searchParams ==null){
-this.#searchParams =newURLSearchParams(this.search);
+this.#searchParams =newURLSearchParams(this.#getSearchFromContext());
 setURLSearchParamsContext(this.#searchParams,this);
+this.#searchParamsModified =false;
 }
 returnthis.#searchParams;
 }
@@ -1033,6 +1091,8 @@ class URL{
 }
 
 toJSON(){
+// Updates to URLSearchParams are lazily propagated to URL, so we need to check we're in sync.
+this.#ensureSearchParamsUpdated();
 returnthis.#context.href;
 }
 

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

@@ -43,6 +43,42 @@ assert.strictEqual(sp.toString(), serialized);
 
 assert.strictEqual(m.search,`?${serialized}`);
 
+sp.delete('a');
+values.forEach((i)=>sp.append('a',i));
+assert.strictEqual(m.href,`http://example.org/?${serialized}`);
+
+sp.delete('a');
+values.forEach((i)=>sp.append('a',i));
+assert.strictEqual(m.toString(),`http://example.org/?${serialized}`);
+
+sp.delete('a');
+values.forEach((i)=>sp.append('a',i));
+assert.strictEqual(m.toJSON(),`http://example.org/?${serialized}`);
+
+sp.delete('a');
+values.forEach((i)=>sp.append('a',i));
+m.href='http://example.org';
+assert.strictEqual(m.href,'http://example.org/');
+assert.strictEqual(sp.size,0);
+
+sp.delete('a');
+values.forEach((i)=>sp.append('a',i));
+m.search='';
+assert.strictEqual(m.href,'http://example.org/');
+assert.strictEqual(sp.size,0);
+
+sp.delete('a');
+values.forEach((i)=>sp.append('a',i));
+m.pathname='/test';
+assert.strictEqual(m.href,`http://example.org/test?${serialized}`);
+m.pathname='';
+
+sp.delete('a');
+values.forEach((i)=>sp.append('a',i));
+m.hash='#test';
+assert.strictEqual(m.href,`http://example.org/?${serialized}#test`);
+m.hash='';
+
 assert.strictEqual(sp[Symbol.iterator],sp.entries);
 
 letkey,val;

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

@@ -11,20 +11,33 @@ const assert = require('assert');
 ].forEach((i)=>{
 assert.throws(()=>Reflect.apply(URL.prototype[i],[],{}),{
 name: 'TypeError',
-message: /Cannotreadprivatemember/,
+message: /Receivermustbeaninstanceofclass/,
 });
 });
 
 [
 'href',
+'search',
+].forEach((i)=>{
+assert.throws(()=>Reflect.get(URL.prototype,i,{}),{
+name: 'TypeError',
+message: /Receivermustbeaninstanceofclass/,
+});
+
+assert.throws(()=>Reflect.set(URL.prototype,i,null,{}),{
+name: 'TypeError',
+message: /Cannotreadprivatemember/,
+});
+});
+
+[
 'protocol',
 'username',
 'password',
 'host',
 'hostname',
 'port',
 'pathname',
-'search',
 'hash',
 ].forEach((i)=>{
 assert.throws(()=>Reflect.get(URL.prototype,i,{}),{