Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FEATURE Router Service] Deprecate touching transition state #17038

Merged
merged 1 commit into from
Oct 4, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@
"puppeteer": "^1.3.0",
"qunit": "^2.5.0",
"route-recognizer": "^0.3.4",
"router_js": "^5.2.0",
"router_js": "^6.0.0",
"rsvp": "^4.8.2",
"semver": "^5.5.0",
"serve-static": "^1.12.2",
Expand Down
21 changes: 14 additions & 7 deletions packages/@ember/-internals/routing/lib/system/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,14 @@ import { once } from '@ember/runloop';
import { classify } from '@ember/string';
import { DEBUG } from '@glimmer/env';
import { TemplateFactory } from '@glimmer/opcode-compiler';
import { InternalRouteInfo, Route as IRoute, Transition, TransitionState } from 'router_js';
import {
InternalRouteInfo,
PARAMS_SYMBOL,
Route as IRoute,
STATE_SYMBOL,
Transition,
TransitionState,
} from 'router_js';
import {
calculateCacheKey,
normalizeControllerQueryParams,
Expand Down Expand Up @@ -219,7 +226,7 @@ class Route extends EmberObject implements IRoute {
}

let transition = this._router._routerMicrolib.activeTransition;
let state = transition ? transition.state : this._router._routerMicrolib.state;
let state = transition ? transition[STATE_SYMBOL] : this._router._routerMicrolib.state;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need svelting here I think

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you might be confused. transition.state is still needed internally but access to it from application code should be deprecated and we just tell people to use the RouteInfo. All of these symbols are just allowing the integration of ember-router and router.js to work. I could have simply renamed the method but I felt that using a faux-symbol may deter people further. In the future we should just make these real symbols.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 thank you!

some of the comments though are specifically about deprecations, and should still apply


let fullName = route.fullRouteName;
let params = assign({}, state!.params[fullName]);
Expand Down Expand Up @@ -899,10 +906,10 @@ class Route extends EmberObject implements IRoute {

if (transition) {
// Update the model dep values used to calculate cache keys.
stashParamNames(this._router, transition.state!.routeInfos);
stashParamNames(this._router, transition[STATE_SYMBOL]!.routeInfos);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

svelting?


let cache = this._bucketCache;
let params = transition.params;
let params = transition[PARAMS_SYMBOL];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Svelting?

let allParams = queryParams.propertyNames;

allParams.forEach((prop: string) => {
Expand All @@ -914,7 +921,7 @@ class Route extends EmberObject implements IRoute {
set(controller, prop, value);
});

let qpValues = getQueryParamsFor(this, transition.state!);
let qpValues = getQueryParamsFor(this, transition[STATE_SYMBOL]!);
setProperties(controller, qpValues);
}

Expand Down Expand Up @@ -1152,7 +1159,7 @@ class Route extends EmberObject implements IRoute {
if (transition.resolveIndex < 1) {
return;
}
return transition.state!.routeInfos[transition.resolveIndex - 1].context;
return transition[STATE_SYMBOL]!.routeInfos[transition.resolveIndex - 1].context;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Svelting? What do we return after the deprecation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No where this is returning the context aka the result of the model hook

}
}

Expand Down Expand Up @@ -2432,7 +2439,7 @@ Route.reopen(ActionHandler, Evented, {
return;
}

let routeInfos = transition.state!.routeInfos;
let routeInfos = transition[STATE_SYMBOL]!.routeInfos;
let router = this._router;
let qpMeta = router._queryParamsFor(routeInfos);
let changes = router._qpUpdates;
Expand Down
61 changes: 57 additions & 4 deletions packages/@ember/-internals/routing/lib/system/router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { getOwner, Owner } from '@ember/-internals/owner';
import { A as emberA, Evented, Object as EmberObject, typeOf } from '@ember/-internals/runtime';
import { EMBER_ROUTING_ROUTER_SERVICE } from '@ember/canary-features';
import { assert, deprecate, info } from '@ember/debug';
import { HANDLER_INFOS, ROUTER_EVENTS } from '@ember/deprecated-features';
import { HANDLER_INFOS, ROUTER_EVENTS, TRANSITION_STATE } from '@ember/deprecated-features';
import EmberError from '@ember/error';
import { assign } from '@ember/polyfills';
import { cancel, once, run, scheduleOnce } from '@ember/runloop';
Expand All @@ -27,6 +27,9 @@ import Router, {
InternalRouteInfo,
InternalTransition,
logAbort,
PARAMS_SYMBOL,
QUERY_PARAMS_SYMBOL,
STATE_SYMBOL,
Transition,
TransitionError,
TransitionState,
Expand Down Expand Up @@ -73,6 +76,56 @@ function defaultWillTransition(
}
}

if (TRANSITION_STATE) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

svelte

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a svelte import.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DOH!

Object.defineProperty(InternalTransition.prototype, 'state', {
get() {
if (EMBER_ROUTING_ROUTER_SERVICE) {
deprecate(
'You attempted to read "transition.state" which is a private API. You should read the `RouteInfo` object on "transition.to" or "transition.from" which has the public state on it.',
false,
{
id: 'transition-state',
until: '3.9.0',
}
);
}
return this[STATE_SYMBOL];
},
});

Object.defineProperty(InternalTransition.prototype, 'queryParams', {
get() {
if (EMBER_ROUTING_ROUTER_SERVICE) {
deprecate(
'You attempted to read "transition.queryParams" which is a private API. You should read the `RouteInfo` object on "transition.to" or "transition.from" which has the queryParams on it.',
false,
{
id: 'transition-state',
until: '3.9.0',
}
);
}
return this[QUERY_PARAMS_SYMBOL];
},
});

Object.defineProperty(InternalTransition.prototype, 'params', {
get() {
if (EMBER_ROUTING_ROUTER_SERVICE) {
deprecate(
'You attempted to read "transition.params" which is a private API. You should read the `RouteInfo` object on "transition.to" or "transition.from" which has the params on it.',
false,
{
id: 'transition-state',
until: '3.9.0',
}
);
}
return this[PARAMS_SYMBOL];
},
});
}

if (HANDLER_INFOS) {
Object.defineProperty(InternalRouteInfo.prototype, 'handler', {
get() {
Expand Down Expand Up @@ -944,7 +997,7 @@ class EmberRouter extends EmberObject {

let unchangedQPs = {};
let qpUpdates = this._qpUpdates;
let params = this._routerMicrolib.activeTransition.queryParams;
let params = this._routerMicrolib.activeTransition[QUERY_PARAMS_SYMBOL];
for (let key in params) {
if (!qpUpdates.has(key)) {
unchangedQPs[key] = params[key];
Expand Down Expand Up @@ -1203,7 +1256,7 @@ class EmberRouter extends EmberObject {
let targetState = new RouterState(
this,
this._routerMicrolib,
this._routerMicrolib.activeTransition.state!
this._routerMicrolib.activeTransition[STATE_SYMBOL]!
);
this.set('targetState', targetState);

Expand Down Expand Up @@ -1668,7 +1721,7 @@ EmberRouter.reopenClass({
});

function didBeginTransition(transition: Transition, router: EmberRouter) {
let routerState = new RouterState(router, router._routerMicrolib, transition.state!);
let routerState = new RouterState(router, router._routerMicrolib, transition[STATE_SYMBOL]!);

if (!router.currentState) {
router.set('currentState', routerState);
Expand Down
4 changes: 2 additions & 2 deletions packages/@ember/-internals/routing/lib/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { get } from '@ember/-internals/metal';
import { getOwner } from '@ember/-internals/owner';
import EmberError from '@ember/error';
import { assign } from '@ember/polyfills';
import Router from 'router_js';
import Router, { STATE_SYMBOL } from 'router_js';
import Route from './system/route';
import EmberRouter, { PrivateRouteInfo, QueryParam } from './system/router';

Expand All @@ -26,7 +26,7 @@ export function extractRouteArgs(args: any[]) {

export function getActiveTargetName(router: Router<Route>) {
let routeInfos = router.activeTransition
? router.activeTransition.state!.routeInfos
? router.activeTransition[STATE_SYMBOL]!.routeInfos
: router.state!.routeInfos;
return routeInfos[routeInfos.length - 1].name;
}
Expand Down
1 change: 1 addition & 0 deletions packages/@ember/deprecated-features/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,4 @@ export const ORDERED_SET = !!'3.3.0-beta.1';
export const MERGE = !!'3.6.0-beta.1';
export const HANDLER_INFOS = !!'3.9.0';
export const ROUTER_EVENTS = !!'3.9.0';
export const TRANSITION_STATE = !!'3.9.0';
45 changes: 45 additions & 0 deletions packages/ember/tests/routing/deprecated_transition_state_test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
import { RouterTestCase, moduleFor } from 'internal-test-helpers';
import { EMBER_ROUTING_ROUTER_SERVICE } from '@ember/canary-features';

if (EMBER_ROUTING_ROUTER_SERVICE) {
moduleFor(
'Deprecated Transition State',
class extends RouterTestCase {
'@test touching transition.state is deprecated'(assert) {
assert.expect(1);
return this.visit('/').then(() => {
this.routerService.on('routeWillChange', transition => {
expectDeprecation(() => {
transition.state;
}, 'You attempted to read "transition.state" which is a private API. You should read the `RouteInfo` object on "transition.to" or "transition.from" which has the public state on it.');
});
return this.routerService.transitionTo('/child');
});
}

'@test touching transition.queryParams is deprecated'(assert) {
assert.expect(1);
return this.visit('/').then(() => {
this.routerService.on('routeWillChange', transition => {
expectDeprecation(() => {
transition.queryParams;
}, 'You attempted to read "transition.queryParams" which is a private API. You should read the `RouteInfo` object on "transition.to" or "transition.from" which has the queryParams on it.');
});
return this.routerService.transitionTo('/child');
});
}

'@test touching transition.params is deprecated'(assert) {
assert.expect(1);
return this.visit('/').then(() => {
this.routerService.on('routeWillChange', transition => {
expectDeprecation(() => {
transition.params;
}, 'You attempted to read "transition.params" which is a private API. You should read the `RouteInfo` object on "transition.to" or "transition.from" which has the params on it.');
});
return this.routerService.transitionTo('/child');
});
}
}
);
}
3 changes: 2 additions & 1 deletion packages/ember/tests/routing/query_params_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { run } from '@ember/runloop';
import { peekMeta } from '@ember/-internals/meta';
import { get, computed } from '@ember/-internals/metal';
import { Route } from '@ember/-internals/routing';
import { PARAMS_SYMBOL } from 'router_js';

import { QueryParamTestCase, moduleFor, getTextOf } from 'internal-test-helpers';

Expand Down Expand Up @@ -1332,7 +1333,7 @@ moduleFor(
'route:other',
Route.extend({
model(p, trans) {
let m = peekMeta(trans.params.application);
let m = peekMeta(trans[PARAMS_SYMBOL].application);
assert.ok(m === undefined, "A meta object isn't constructed for this params POJO");
},
})
Expand Down
8 changes: 4 additions & 4 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -7385,10 +7385,10 @@ route-recognizer@^0.3.4:
resolved "https://registry.yarnpkg.com/route-recognizer/-/route-recognizer-0.3.4.tgz#39ab1ffbce1c59e6d2bdca416f0932611e4f3ca3"
integrity sha512-2+MhsfPhvauN1O8KaXpXAOfR/fwe8dnUXVM+xw7yt40lJRfPVQxV6yryZm0cgRvAj5fMF/mdRZbL2ptwbs5i2g==

router_js@^5.2.0:
version "5.2.0"
resolved "https://registry.yarnpkg.com/router_js/-/router_js-5.2.0.tgz#8796d0ad7ab8a9d0ffbf5b02e5e00d2472a53e7d"
integrity sha512-v+gjYRwDWJpJW0jPB9tFphbcp0pD7R/ZRqu/tno9TXgQxanRArw/weyGFZnbpR95tY9B5SpFonAZk5opPNQUvQ==
router_js@^6.0.0:
version "6.0.0"
resolved "https://registry.yarnpkg.com/router_js/-/router_js-6.0.0.tgz#a9dd8918987f0464d31cfb409d8ea1496bf38ba2"
integrity sha512-2P0Be0+IMnYwLfv7SDsRs7PZCs5LWF1FTy4/m8FB5NdCCmIViCb829cfVYAdx40oeoHtRho+hiVUDWv+czqF9A==
dependencies:
"@types/node" "^10.5.5"

Expand Down