Skip to content

Commit

Permalink
Rename private property Route#router to Router#_router
Browse files Browse the repository at this point in the history
The fact that this private property was named router was preventing user
from injecting the router service into routes like this:

```js
export default Route.extend({
  router: service()
});
```

I consider that the above code is pretty reasonable, and it should be
the private property the one that should be renamed to a clearer `_router`
name that is less likely to collide with user-defined properties or methods.

If you deem this an _intimate_ API we could consider adding a deprecated
alias, but I don't have evidence to consider it such, as `router` is too
common of a term to search for in emberobserver.

(cherry picked from commit 4690f63)
  • Loading branch information
cibernox authored and kategengler committed Apr 10, 2018
1 parent 96ed878 commit ad09111
Show file tree
Hide file tree
Showing 7 changed files with 58 additions and 41 deletions.
2 changes: 1 addition & 1 deletion packages/ember-application/lib/system/engine.js
Original file line number Diff line number Diff line change
Expand Up @@ -502,7 +502,7 @@ function commonSetupRegistry(registry) {
registry.injection('router', '_bucketCache', P`-bucket-cache:main`);
registry.injection('route', '_bucketCache', P`-bucket-cache:main`);

registry.injection('route', 'router', 'router:main');
registry.injection('route', '_router', 'router:main');

// Register the routing service...
registry.register('service:-routing', RoutingService);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ moduleFor('Application', class extends ApplicationTestCase {
verifyInjection(assert, application, 'router', '_bucketCache', P`-bucket-cache:main`);
verifyInjection(assert, application, 'route', '_bucketCache', P`-bucket-cache:main`);

verifyInjection(assert, application, 'route', 'router', 'router:main');
verifyInjection(assert, application, 'route', '_router', 'router:main');

verifyRegistration(assert, application, 'component:-text-field');
verifyRegistration(assert, application, 'component:-text-area');
Expand Down
2 changes: 1 addition & 1 deletion packages/ember-application/tests/system/engine_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ moduleFor('Engine', class extends TestCase {
verifyInjection(assert, engine, 'router', '_bucketCache', P`-bucket-cache:main`);
verifyInjection(assert, engine, 'route', '_bucketCache', P`-bucket-cache:main`);

verifyInjection(assert, engine, 'route', 'router', 'router:main');
verifyInjection(assert, engine, 'route', '_router', 'router:main');

verifyRegistration(assert, engine, 'component:-text-field');
verifyRegistration(assert, engine, 'component:-text-area');
Expand Down
59 changes: 34 additions & 25 deletions packages/ember-routing/lib/system/route.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {
run,
isEmpty
} from 'ember-metal';
import { assert, info, isTesting } from 'ember-debug';
import { assert, info, isTesting, deprecate } from 'ember-debug';
import { DEBUG } from 'ember-env-flags';
import {
typeOf,
Expand Down Expand Up @@ -117,6 +117,15 @@ let Route = EmberObject.extend(ActionHandler, Evented, {
*/
queryParams: {},

router: computed('_router', function() {
deprecate(
'Route#router is an intimate API that has been renamed to Route#_router. However you might want to consider using the router service',
false,
{ id: 'ember-routing.route-router', until: '3.5.0', url: 'https://emberjs.com/deprecations/v3.x#toc_ember-routing-route-router' }
);
return this._router;
}),

/**
The name of the route, dot-delimited.
Expand Down Expand Up @@ -307,15 +316,15 @@ let Route = EmberObject.extend(ActionHandler, Evented, {
@property _activeQPChanged
*/
_activeQPChanged(qp, value) {
this.router._activeQPChanged(qp.scopedPropertyName, value);
this._router._activeQPChanged(qp.scopedPropertyName, value);
},

/**
@private
@method _updatingQPChanged
*/
_updatingQPChanged(qp) {
this.router._updatingQPChanged(qp.urlKey);
this._router._updatingQPChanged(qp.urlKey);
},

mergedProperties: ['queryParams'],
Expand Down Expand Up @@ -378,8 +387,8 @@ let Route = EmberObject.extend(ActionHandler, Evented, {
return {};
}

let transition = this.router._routerMicrolib.activeTransition;
let state = transition ? transition.state : this.router._routerMicrolib.state;
let transition = this._router._routerMicrolib.activeTransition;
let state = transition ? transition.state : this._router._routerMicrolib.state;

let fullName = route.fullRouteName;
let params = assign({}, state.params[fullName]);
Expand Down Expand Up @@ -416,7 +425,7 @@ let Route = EmberObject.extend(ActionHandler, Evented, {
// urlKey isn't used here, but anyone overriding
// can use it to provide serialization specific
// to a certain query param.
return this.router._serializeQueryParam(value, defaultValueType);
return this._router._serializeQueryParam(value, defaultValueType);
},

/**
Expand All @@ -432,7 +441,7 @@ let Route = EmberObject.extend(ActionHandler, Evented, {
// urlKey isn't used here, but anyone overriding
// can use it to provide deserialization specific
// to a certain query param.
return this.router._deserializeQueryParam(value, defaultValueType);
return this._router._deserializeQueryParam(value, defaultValueType);
},

/**
Expand Down Expand Up @@ -807,7 +816,7 @@ let Route = EmberObject.extend(ActionHandler, Evented, {
let totalChanged = Object.keys(changed).concat(Object.keys(removed));
for (let i = 0; i < totalChanged.length; ++i) {
let qp = qpMap[totalChanged[i]];
if (qp && get(this._optionsForQueryParam(qp), 'refreshModel') && this.router.currentState) {
if (qp && get(this._optionsForQueryParam(qp), 'refreshModel') && this._router.currentState) {
this.refresh();
break;
}
Expand All @@ -823,7 +832,7 @@ let Route = EmberObject.extend(ActionHandler, Evented, {
if (!transition) { return; }

let handlerInfos = transition.state.handlerInfos;
let router = this.router;
let router = this._router;
let qpMeta = router._queryParamsFor(handlerInfos);
let changes = router._qpUpdates;
let replaceUrl;
Expand Down Expand Up @@ -1118,7 +1127,7 @@ let Route = EmberObject.extend(ActionHandler, Evented, {
@public
*/
transitionTo(name, context) { // eslint-disable-line no-unused-vars
return this.router.transitionTo(...prefixRouteNameArg(this, arguments));
return this._router.transitionTo(...prefixRouteNameArg(this, arguments));
},

/**
Expand All @@ -1139,7 +1148,7 @@ let Route = EmberObject.extend(ActionHandler, Evented, {
@public
*/
intermediateTransitionTo() {
this.router.intermediateTransitionTo(...prefixRouteNameArg(this, arguments));
this._router.intermediateTransitionTo(...prefixRouteNameArg(this, arguments));
},

/**
Expand All @@ -1165,7 +1174,7 @@ let Route = EmberObject.extend(ActionHandler, Evented, {
@public
*/
refresh() {
return this.router._routerMicrolib.refresh(this);
return this._router._routerMicrolib.refresh(this);
},

/**
Expand Down Expand Up @@ -1211,7 +1220,7 @@ let Route = EmberObject.extend(ActionHandler, Evented, {
@public
*/
replaceWith() {
return this.router.replaceWith(...prefixRouteNameArg(this, arguments));
return this._router.replaceWith(...prefixRouteNameArg(this, arguments));
},

/**
Expand Down Expand Up @@ -1263,8 +1272,8 @@ let Route = EmberObject.extend(ActionHandler, Evented, {
@public
*/
send(...args) {
if ((this.router && this.router._routerMicrolib) || !isTesting()) {
this.router.send(...args);
if ((this._router && this._router._routerMicrolib) || !isTesting()) {
this._router.send(...args);
} else {
let name = args.shift();
let action = this.actions[name];
Expand Down Expand Up @@ -1308,7 +1317,7 @@ let Route = EmberObject.extend(ActionHandler, Evented, {

if (transition) {
// Update the model dep values used to calculate cache keys.
stashParamNames(this.router, transition.state.handlerInfos);
stashParamNames(this._router, transition.state.handlerInfos);

let cache = this._bucketCache;
let params = transition.params;
Expand Down Expand Up @@ -1606,7 +1615,7 @@ let Route = EmberObject.extend(ActionHandler, Evented, {
store: computed(function() {
let owner = getOwner(this);
let routeName = this.routeName;
let namespace = get(this, 'router.namespace');
let namespace = get(this, '_router.namespace');

return {
find(name, value) {
Expand Down Expand Up @@ -1865,7 +1874,7 @@ let Route = EmberObject.extend(ActionHandler, Evented, {
modelFor(_name) {
let name;
let owner = getOwner(this);
let transition = this.router ? this.router._routerMicrolib.activeTransition : null;
let transition = this._router ? this._router._routerMicrolib.activeTransition : null;

// Only change the route name when there is an active transition.
// Otherwise, use the passed in route name.
Expand Down Expand Up @@ -2069,7 +2078,7 @@ let Route = EmberObject.extend(ActionHandler, Evented, {

let renderOptions = buildRenderOptions(this, isDefaultRender, name, options);
this.connections.push(renderOptions);
run.once(this.router, '_setOutlets');
run.once(this._router, '_setOutlets');
},

/**
Expand Down Expand Up @@ -2148,7 +2157,7 @@ let Route = EmberObject.extend(ActionHandler, Evented, {

outletName = outletName || 'main';
this._disconnectOutlet(outletName, parentView);
let handlerInfos = this.router._routerMicrolib.currentHandlerInfos;
let handlerInfos = this._router._routerMicrolib.currentHandlerInfos;
for (let i = 0; i < handlerInfos.length; i++) {
// This non-local state munging is sadly necessary to maintain
// backward compatibility with our existing semantics, which allow
Expand Down Expand Up @@ -2180,7 +2189,7 @@ let Route = EmberObject.extend(ActionHandler, Evented, {
controller: undefined,
template: undefined,
};
run.once(this.router, '_setOutlets');
run.once(this._router, '_setOutlets');
}
}
},
Expand All @@ -2197,7 +2206,7 @@ let Route = EmberObject.extend(ActionHandler, Evented, {
teardownViews() {
if (this.connections && this.connections.length > 0) {
this.connections = [];
run.once(this.router, '_setOutlets');
run.once(this._router, '_setOutlets');
}
}
});
Expand All @@ -2207,7 +2216,7 @@ Route.reopenClass({
});

function parentRoute(route) {
let handlerInfo = handlerInfoFor(route, route.router._routerMicrolib.state.handlerInfos, -1);
let handlerInfo = handlerInfoFor(route, route._router._routerMicrolib.state.handlerInfos, -1);
return handlerInfo && handlerInfo.handler;
}

Expand Down Expand Up @@ -2284,7 +2293,7 @@ function buildRenderOptions(route, isDefaultRender, _name, options) {
};

if (DEBUG) {
let LOG_VIEW_LOOKUPS = get(route.router, 'namespace.LOG_VIEW_LOOKUPS');
let LOG_VIEW_LOOKUPS = get(route._router, 'namespace.LOG_VIEW_LOOKUPS');
if (LOG_VIEW_LOOKUPS && !template) {
info(`Could not find "${name}" template. Nothing will be rendered`, { fullName: `template:${name}` });
}
Expand All @@ -2309,7 +2318,7 @@ function getQueryParamsFor(route, state) {

if (state.queryParamsFor[name]) { return state.queryParamsFor[name]; }

let fullQueryParams = getFullQueryParams(route.router, state);
let fullQueryParams = getFullQueryParams(route._router, state);

let params = state.queryParamsFor[name] = {};

Expand Down
6 changes: 3 additions & 3 deletions packages/ember-routing/lib/system/router.js
Original file line number Diff line number Diff line change
Expand Up @@ -1195,7 +1195,7 @@ function logError(_error, initialMessage) {
*/
function findRouteSubstateName(route, state) {
let owner = getOwner(route);
let { routeName, fullRouteName, router } = route;
let { routeName, fullRouteName, _router: router } = route;

let substateName = `${routeName}_${state}`;
let substateNameFull = `${fullRouteName}_${state}`;
Expand All @@ -1217,7 +1217,7 @@ function findRouteSubstateName(route, state) {
*/
function findRouteStateName(route, state) {
let owner = getOwner(route);
let { routeName, fullRouteName, router } = route;
let { routeName, fullRouteName, _router: router } = route;

let stateName = routeName === 'application' ? state : `${routeName}.${state}`;
let stateNameFull = fullRouteName === 'application' ? state : `${fullRouteName}.${state}`;
Expand Down Expand Up @@ -1265,7 +1265,7 @@ export function triggerEvent(handlerInfos, ignoreFailure, args) {
} else {
// Should only hit here if a non-bubbling error action is triggered on a route.
if (name === 'error') {
handler.router._markErrorAsHandled(args[0]);
handler._router._markErrorAsHandled(args[0]);
}
return;
}
Expand Down
26 changes: 17 additions & 9 deletions packages/ember-routing/tests/system/route_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -327,9 +327,9 @@ moduleFor('Route with engines', class extends AbstractTestCase {
}
});

let applicationRoute = EmberRoute.create({ router, routeName: 'application', fullRouteName: 'foo.bar' });
let postsRoute = EmberRoute.create({ router, routeName: 'posts', fullRouteName: 'foo.bar.posts' });
let route = EmberRoute.create({ router });
let applicationRoute = EmberRoute.create({ _router: router, routeName: 'application', fullRouteName: 'foo.bar' });
let postsRoute = EmberRoute.create({ _router: router, routeName: 'posts', fullRouteName: 'foo.bar.posts' });
let route = EmberRoute.create({ _router: router });

setOwner(applicationRoute, engineInstance);
setOwner(postsRoute, engineInstance);
Expand Down Expand Up @@ -370,9 +370,9 @@ moduleFor('Route with engines', class extends AbstractTestCase {
}
});

let applicationRoute = EmberRoute.create({ router, routeName: 'application' });
let postsRoute = EmberRoute.create({ router, routeName: 'posts' });
let route = EmberRoute.create({ router });
let applicationRoute = EmberRoute.create({ _router: router, routeName: 'application' });
let postsRoute = EmberRoute.create({ _router: router, routeName: 'posts' });
let route = EmberRoute.create({ _router: router });

setOwner(applicationRoute, engineInstance);
setOwner(postsRoute, engineInstance);
Expand All @@ -396,7 +396,7 @@ moduleFor('Route with engines', class extends AbstractTestCase {
}
});

let route = EmberRoute.create({ router });
let route = EmberRoute.create({ _router: router });
setOwner(route, engineInstance);

assert.strictEqual(route.transitionTo('application'), 'foo.bar.application', 'properly prefixes application route');
Expand All @@ -422,7 +422,7 @@ moduleFor('Route with engines', class extends AbstractTestCase {
}
});

let route = EmberRoute.create({ router });
let route = EmberRoute.create({ _router: router });
setOwner(route, engineInstance);

route.intermediateTransitionTo('application');
Expand Down Expand Up @@ -452,7 +452,7 @@ moduleFor('Route with engines', class extends AbstractTestCase {
}
});

let route = EmberRoute.create({ router });
let route = EmberRoute.create({ _router: router });
setOwner(route, engineInstance);

assert.strictEqual(route.replaceWith('application'), 'foo.bar.application', 'properly prefixes application route');
Expand All @@ -462,4 +462,12 @@ moduleFor('Route with engines', class extends AbstractTestCase {
let queryParams = {};
assert.strictEqual(route.replaceWith(queryParams), queryParams, 'passes query param only transitions through');
}

['@test `router` is a deprecated one-way alias to `_router`'](assert) {
let router = {};
let route = EmberRoute.create({ _router: router });
expectDeprecation(function() {
return assert.equal(route.router, router);
}, 'Route#router is an intimate API that has been renamed to Route#_router. However you might want to consider using the router service');
}
});
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ if (EMBER_ROUTING_ROUTER_SERVICE) {
this.add('route:parent.index', Route.extend({
init() {
this._super();
set(this.router, 'rootURL', '/homepage');
set(this._router, 'rootURL', '/homepage');
}
}));

Expand Down

0 comments on commit ad09111

Please sign in to comment.