[BUGFIX] Don't serialize default QPs on RouterService

Fixes #19493 (cherry picked from commit 6416faf)
emberjs · Mar 1, 2022 · 5a83504 · 5a83504
1 parent cb9b164
commit 5a83504
Show file tree

Hide file tree

Showing 6 changed files with 32 additions and 27 deletions.
diff --git a/packages/@ember/-internals/routing/lib/services/router.ts b/packages/@ember/-internals/routing/lib/services/router.ts
@@ -129,7 +129,6 @@ class RouterService<R extends Route> extends Service.extend(Evented) {
     let { routeName, models, queryParams } = extractRouteArgs(args);
 
     let transition = this._router._doTransition(routeName, models, queryParams, true);
-    transition['_keepDefaultQueryParamValues'] = true;
 
     return transition;
   }
@@ -320,21 +319,31 @@ class RouterService<R extends Route> extends Service.extend(Evented) {
     let hasQueryParams = Object.keys(queryParams).length > 0;
 
     if (hasQueryParams) {
+      // UNSAFE: casting `routeName as string` here encodes the existing
+      // assumption but may be wrong: `extractRouteArgs` correctly returns it
+      // as `string | undefined`. There may be bugs if `_prepareQueryParams`
+      // does not correctly account for `undefined` values for `routeName`.
+      //  Spoilers: under the hood this currently uses router.js APIs which
+      // *do not* account for this being `undefined`.
+      let targetRouteName = routeName as string;
+
       queryParams = Object.assign({}, queryParams);
       this._router._prepareQueryParams(
-        // UNSAFE: casting `routeName as string` here encodes the existing
-        // assumption but may be wrong: `extractRouteArgs` correctly returns it
-        // as `string | undefined`. There may be bugs if `_prepareQueryParams`
-        // does not correctly account for `undefined` values for `routeName`.
-        //  Spoilers: under the hood this currently uses router.js APIs which
-        // *do not* account for this being `undefined`.
-        routeName as string,
+        targetRouteName,
         models,
         queryParams,
         true /* fromRouterService */
       );
 
-      return shallowEqual(queryParams, routerMicrolib.state!.queryParams);
+      let currentQueryParams = Object.assign({}, routerMicrolib.state!.queryParams);
+      this._router._prepareQueryParams(
+        targetRouteName,
+        models,
+        currentQueryParams,
+        true /* fromRouterService */
+      );
+
+      return shallowEqual(queryParams, currentQueryParams);
     }
 
     return true;

diff --git a/packages/@ember/-internals/routing/lib/system/route.ts b/packages/@ember/-internals/routing/lib/system/route.ts
@@ -2515,7 +2515,7 @@ Route.reopen({
         qp.serializedValue = svalue;
 
         let thisQueryParamHasDefaultValue = qp.serializedDefaultValue === svalue;
-        if (!thisQueryParamHasDefaultValue || (transition as any)._keepDefaultQueryParamValues) {
+        if (!thisQueryParamHasDefaultValue) {
           finalParams.push({
             value: svalue,
             visible: true,

diff --git a/packages/@ember/-internals/routing/lib/system/router.ts b/packages/@ember/-internals/routing/lib/system/router.ts
@@ -1058,7 +1058,7 @@ class EmberRouter<R extends Route = Route> extends EmberObject.extend(Evented) i
     _targetRouteName: string | undefined,
     models: ModelFor<R>[],
     _queryParams: Record<string, unknown>,
-    _keepDefaultQueryParamValues?: boolean
+    _fromRouterService?: boolean
   ): Transition {
     let targetRouteName = _targetRouteName || getActiveTargetName(this._routerMicrolib);
     assert(
@@ -1074,12 +1074,7 @@ class EmberRouter<R extends Route = Route> extends EmberObject.extend(Evented) i
 
     Object.assign(queryParams, _queryParams);
 
-    this._prepareQueryParams(
-      targetRouteName,
-      models,
-      queryParams,
-      Boolean(_keepDefaultQueryParamValues)
-    );
+    this._prepareQueryParams(targetRouteName, models, queryParams, Boolean(_fromRouterService));
 
     let transition = this._routerMicrolib.transitionTo(targetRouteName, ...models, { queryParams });
 

diff --git a/packages/ember/tests/routing/router_service_test/isActive_test.js b/packages/ember/tests/routing/router_service_test/isActive_test.js
@@ -117,9 +117,10 @@ moduleFor(
           return this.routerService.transitionTo('parent.child', queryParams);
         })
         .then(() => {
-          assert.ok(this.routerService.isActive('parent.child', queryParams));
+          assert.ok(this.routerService.isActive('parent.child', queryParams), 'route is active');
           assert.notOk(
-            this.routerService.isActive('parent.child', this.buildQueryParams({ sort: 'DESC' }))
+            this.routerService.isActive('parent.child', this.buildQueryParams({ sort: 'DESC' })),
+            'route with QPs is not active'
           );
         });
     }

diff --git a/packages/ember/tests/routing/router_service_test/replaceWith_test.js b/packages/ember/tests/routing/router_service_test/replaceWith_test.js
@@ -107,7 +107,7 @@ moduleFor(
         });
     }
 
-    ['@test RouterService#replaceWith with basic query params does not remove query param defaults'](
+    ['@test RouterService#replaceWith with basic query params removes query param defaults'](
       assert
     ) {
       assert.expect(1);
@@ -133,7 +133,7 @@ moduleFor(
           return this.routerService.replaceWith('parent.child', queryParams);
         })
         .then(() => {
-          assert.deepEqual(this.state, ['/', '/child?sort=ASC']);
+          assert.deepEqual(this.state, ['/', '/child']);
         });
     }
   }

diff --git a/packages/ember/tests/routing/router_service_test/transitionTo_test.js b/packages/ember/tests/routing/router_service_test/transitionTo_test.js
@@ -238,7 +238,7 @@ moduleFor(
       this.assertText('much dynamicism');
     }
 
-    ['@test RouterService#transitionTo with basic query params does not remove query param defaults'](
+    ['@test RouterService#transitionTo with basic query params removes query param defaults'](
       assert
     ) {
       assert.expect(1);
@@ -258,7 +258,7 @@ moduleFor(
           return this.routerService.transitionTo('parent.child', queryParams);
         })
         .then(() => {
-          assert.equal(this.routerService.get('currentURL'), '/child?sort=ASC');
+          assert.equal(this.routerService.get('currentURL'), '/child');
         });
     }
 
@@ -302,14 +302,14 @@ moduleFor(
         })
       );
 
-      let queryParams = this.buildQueryParams({ sort: 'ASC' });
+      let queryParams = this.buildQueryParams({ sort: 'DESC' });
 
       return this.visit('/')
         .then(() => {
           return this.routerService.transitionTo('parent.child', queryParams);
         })
         .then(() => {
-          assert.equal(this.routerService.get('currentURL'), '/child?sort=ASC');
+          assert.equal(this.routerService.get('currentURL'), '/child?sort=DESC');
         });
     }
 
@@ -328,14 +328,14 @@ moduleFor(
         })
       );
 
-      let queryParams = this.buildQueryParams({ url_sort: 'ASC' });
+      let queryParams = this.buildQueryParams({ url_sort: 'DESC' });
 
       return this.visit('/')
         .then(() => {
           return this.routerService.transitionTo('parent.child', queryParams);
         })
         .then(() => {
-          assert.equal(this.routerService.get('currentURL'), '/child?url_sort=ASC');
+          assert.equal(this.routerService.get('currentURL'), '/child?url_sort=DESC');
         });
     }