Skip to content

Commit

Permalink
[BUGFIX] Ensure QP definitions interop with tracked props (#18358)
Browse files Browse the repository at this point in the history
[BUGFIX] Ensure QP definitions interop with tracked props
  • Loading branch information
Chris Garrett authored Sep 13, 2019
2 parents 708b2eb + c2ca5e8 commit 0efdae9
Show file tree
Hide file tree
Showing 4 changed files with 120 additions and 10 deletions.
12 changes: 9 additions & 3 deletions packages/@ember/-internals/metal/lib/observer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -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();
}
}
});
});
Expand Down
38 changes: 37 additions & 1 deletion packages/@ember/-internals/routing/lib/system/route.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
import { OwnedTemplate, TemplateFactory } from '@ember/-internals/glimmer';
import {
addObserver,
computed,
defineProperty,
descriptorForProperty,
flushAsyncObservers,
get,
getProperties,
isEmpty,
Expand All @@ -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';
Expand Down Expand Up @@ -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);
}
}

/*
Expand Down Expand Up @@ -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);
});
}

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -2543,6 +2571,8 @@ Route.reopen(ActionHandler, Evented, {
}

set(controller, qp.prop, value);

qpUpdated = true;
}

// Stash current serialized value of controller.
Expand All @@ -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');
}
Expand Down
12 changes: 7 additions & 5 deletions packages/@ember/-internals/runtime/lib/mixins/observable.js
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,7 @@ export default Mixin.create({
observer should be prepared to handle that.
There are two common invocation patterns for `.addObserver()`:
- Passing two arguments:
- the name of the property to observe (as a string)
- the function to invoke (an actual function)
Expand Down Expand Up @@ -362,11 +362,12 @@ export default Mixin.create({
@param {String} key The key to observe
@param {Object} target The target object to invoke
@param {String|Function} method The method to invoke
@param {Boolean} async Whether the observer is async or not
@return {Observable}
@public
*/
addObserver(key, target, method) {
addObserver(this, key, target, method);
addObserver(key, target, method, async) {
addObserver(this, key, target, method, async);
return this;
},

Expand All @@ -379,11 +380,12 @@ export default Mixin.create({
@param {String} key The key to observe
@param {Object} target The target object to invoke
@param {String|Function} method The method to invoke
@param {Boolean} async Whether the observer is async or not
@return {Observable}
@public
*/
removeObserver(key, target, method) {
removeObserver(this, key, target, method);
removeObserver(key, target, method, async) {
removeObserver(this, key, target, method, async);
return this;
},

Expand Down
68 changes: 67 additions & 1 deletion packages/ember/tests/routing/query_params_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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');
}
}
);

0 comments on commit 0efdae9

Please sign in to comment.