From 7f29c04d1c838a6232491809882dfc92316ef1ea Mon Sep 17 00:00:00 2001 From: Chris Garrett Date: Thu, 5 Sep 2019 15:17:23 -0700 Subject: [PATCH] [BUGFIX] Ensure QP definitions interop with tracked props This PR ensures that QPs can autotrack getters/setters and tracked props. It includes a couple of manual flushes of async observers in order to support legacy sync behaviors of QPs. --- .../@ember/-internals/metal/lib/observer.ts | 12 +++- .../-internals/routing/lib/system/route.ts | 38 ++++++++++- .../ember/tests/routing/query_params_test.js | 68 ++++++++++++++++++- 3 files changed, 113 insertions(+), 5 deletions(-) diff --git a/packages/@ember/-internals/metal/lib/observer.ts b/packages/@ember/-internals/metal/lib/observer.ts index e7aad4355d6..8ecc57c4f86 100644 --- a/packages/@ember/-internals/metal/lib/observer.ts +++ b/packages/@ember/-internals/metal/lib/observer.ts @@ -162,7 +162,7 @@ export function revalidateObservers(target: object) { let lastKnownRevision = 0; -export function flushAsyncObservers() { +export function flushAsyncObservers(shouldSchedule = true) { if (lastKnownRevision === value(CURRENT_TAG)) { return; } @@ -179,14 +179,20 @@ export function flushAsyncObservers() { activeObservers.forEach((observer, eventName) => { if (!validate(observer.tag, observer.lastRevision)) { - schedule('actions', () => { + let sendObserver = () => { try { sendEvent(target, eventName, [target, observer.path]); } finally { observer.tag = combine(getChainTagsForKey(target, observer.path)); observer.lastRevision = value(observer.tag); } - }); + }; + + if (shouldSchedule) { + schedule('actions', sendObserver); + } else { + sendObserver(); + } } }); }); diff --git a/packages/@ember/-internals/routing/lib/system/route.ts b/packages/@ember/-internals/routing/lib/system/route.ts index b3163c09ef0..79b14a9f903 100644 --- a/packages/@ember/-internals/routing/lib/system/route.ts +++ b/packages/@ember/-internals/routing/lib/system/route.ts @@ -1,7 +1,10 @@ import { OwnedTemplate, TemplateFactory } from '@ember/-internals/glimmer'; import { + addObserver, computed, defineProperty, + descriptorForProperty, + flushAsyncObservers, get, getProperties, isEmpty, @@ -17,9 +20,12 @@ import { setFrameworkClass, typeOf, } from '@ember/-internals/runtime'; +import { lookupDescriptor } from '@ember/-internals/utils'; +import { EMBER_METAL_TRACKED_PROPERTIES } from '@ember/canary-features'; import Controller from '@ember/controller'; import { assert, deprecate, info, isTesting } from '@ember/debug'; import { ROUTER_EVENTS } from '@ember/deprecated-features'; +import { dependentKeyCompat } from '@ember/object/compat'; import { assign } from '@ember/polyfills'; import { once } from '@ember/runloop'; import { classify } from '@ember/string'; @@ -941,6 +947,12 @@ class Route extends EmberObject implements IRoute { if (this._environment.options.shouldRender) { this.renderTemplate(controller, context); } + + // Setup can cause changes to QPs which need to be propogated immediately in + // some situations. Eventually, we should work on making these async somehow. + if (EMBER_METAL_TRACKED_PROPERTIES) { + flushAsyncObservers(false); + } } /* @@ -2005,7 +2017,22 @@ function mergeEachQueryParams(controllerQP: {}, routeQP: {}) { function addQueryParamsObservers(controller: any, propNames: string[]) { propNames.forEach(prop => { - controller.addObserver(`${prop}.[]`, controller, controller._qpChanged); + if (descriptorForProperty(controller, prop) === undefined) { + let desc = lookupDescriptor(controller, prop); + + if (desc !== null && (typeof desc.get === 'function' || typeof desc.set === 'function')) { + defineProperty( + controller, + prop, + dependentKeyCompat({ + get: desc.get, + set: desc.set, + }) + ); + } + } + + addObserver(controller, `${prop}.[]`, controller, controller._qpChanged, false); }); } @@ -2495,6 +2522,7 @@ Route.reopen(ActionHandler, Evented, { let router = this._router; let qpMeta = router._queryParamsFor(routeInfos); let changes = router._qpUpdates; + let qpUpdated = false; let replaceUrl; stashParamNames(router, routeInfos); @@ -2543,6 +2571,8 @@ Route.reopen(ActionHandler, Evented, { } set(controller, qp.prop, value); + + qpUpdated = true; } // Stash current serialized value of controller. @@ -2558,6 +2588,12 @@ Route.reopen(ActionHandler, Evented, { } } + // Some QPs have been updated, and those changes need to be propogated + // immediately. Eventually, we should work on making this async somehow. + if (EMBER_METAL_TRACKED_PROPERTIES && qpUpdated === true) { + flushAsyncObservers(false); + } + if (replaceUrl) { transition.method('replace'); } diff --git a/packages/ember/tests/routing/query_params_test.js b/packages/ember/tests/routing/query_params_test.js index b02de2f133c..4824d612e4b 100644 --- a/packages/ember/tests/routing/query_params_test.js +++ b/packages/ember/tests/routing/query_params_test.js @@ -3,7 +3,7 @@ import { dasherize } from '@ember/string'; import { RSVP, Object as EmberObject, A as emberA } from '@ember/-internals/runtime'; import { run } from '@ember/runloop'; import { peekMeta } from '@ember/-internals/meta'; -import { get, computed } from '@ember/-internals/metal'; +import { get, computed, tracked } from '@ember/-internals/metal'; import { Route } from '@ember/-internals/routing'; import { PARAMS_SYMBOL } from 'router_js'; @@ -1581,5 +1581,71 @@ moduleFor( let controller = this.getController('constructor'); assert.equal(get(controller, 'foo'), '999'); } + + async ['@test Single query params defined with tracked properties can be on the controller and reflected in the url']( + assert + ) { + assert.expect(3); + + this.router.map(function() { + this.route('home', { path: '/' }); + }); + + this.add( + `controller:home`, + Controller.extend({ + queryParams: ['foo'], + foo: tracked(), + }) + ); + + await this.visitAndAssert('/'); + let controller = this.getController('home'); + + controller.foo = '456'; + await runLoopSettled(); + this.assertCurrentPath('/?foo=456'); + + controller.foo = '987'; + await runLoopSettled(); + this.assertCurrentPath('/?foo=987'); + } + + async ['@test Single query params defined with native getters and tracked properties can be on the controller and reflected in the url']( + assert + ) { + assert.expect(3); + + this.router.map(function() { + this.route('home', { path: '/' }); + }); + + this.add( + `controller:home`, + Controller.extend({ + queryParams: ['foo'], + get foo() { + return this.bar; + }, + + set foo(value) { + this.bar = value; + }, + + bar: tracked(), + }) + ); + + await this.visitAndAssert('/'); + let controller = this.getController('home'); + + controller.bar = '456'; + await runLoopSettled(); + this.assertCurrentPath('/?foo=456'); + + controller.bar = '987'; + await runLoopSettled(); + this.assertCurrentPath('/?foo=987'); + } } );