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

Lazily setup the router in non-application tests #19080

Merged
merged 6 commits into from
Oct 16, 2020
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
25 changes: 20 additions & 5 deletions packages/@ember/-internals/routing/lib/services/router.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import { getOwner, Owner } from '@ember/-internals/owner';
import { Evented } from '@ember/-internals/runtime';
import { symbol } from '@ember/-internals/utils';
import { assert } from '@ember/debug';
import { readOnly } from '@ember/object/computed';
import { assign } from '@ember/polyfills';
Expand All @@ -9,6 +11,8 @@ import { Transition } from 'router_js';
import EmberRouter, { QueryParam } from '../system/router';
import { extractRouteArgs, resemblesURL, shallowEqual } from '../utils';

const ROUTER = symbol('ROUTER') as string;
rwjblue marked this conversation as resolved.
Show resolved Hide resolved

let freezeRouteInfo: Function;
if (DEBUG) {
freezeRouteInfo = (transition: Transition) => {
Expand Down Expand Up @@ -62,19 +66,30 @@ function cleanURL(url: string, rootURL: string) {
@class RouterService
*/
export default class RouterService extends Service {
_router!: EmberRouter;
get _router(): EmberRouter {
let router = this[ROUTER];
if (router !== undefined) {
return router;
}
const owner = getOwner(this) as Owner;
router = owner.lookup('router:main') as EmberRouter;
router.setupRouter();
rwjblue marked this conversation as resolved.
Show resolved Hide resolved
return (this[ROUTER] = router);
}

constructor(owner: Owner) {
super(owner);

init() {
super.init(...arguments);
const router = owner.lookup('router:main') as EmberRouter;
xg-wang marked this conversation as resolved.
Show resolved Hide resolved

this._router.on('routeWillChange', (transition: Transition) => {
router.on('routeWillChange', (transition: Transition) => {
if (DEBUG) {
freezeRouteInfo(transition);
}
this.trigger('routeWillChange', transition);
});

this._router.on('routeDidChange', (transition: Transition) => {
router.on('routeDidChange', (transition: Transition) => {
if (DEBUG) {
freezeRouteInfo(transition);
}
Expand Down
13 changes: 10 additions & 3 deletions packages/@ember/-internals/routing/lib/system/router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ class EmberRouter extends EmberObject {
location!: string | IEmberLocation;
rootURL!: string;
_routerMicrolib!: Router<Route>;
_didSetupRouter = false;

currentURL: string | null = null;
currentRouteName: string | null = null;
Expand Down Expand Up @@ -397,9 +398,8 @@ class EmberRouter extends EmberObject {
@private
*/
startRouting() {
let initialURL = get(this, 'initialURL');

if (this.setupRouter()) {
let initialURL = get(this, 'initialURL');
if (initialURL === undefined) {
initialURL = get(this, 'location').getURL();
}
Expand All @@ -411,6 +411,10 @@ class EmberRouter extends EmberObject {
}

setupRouter() {
if (this._didSetupRouter) {
return false;
}
this._didSetupRouter = true;
this._setupLocation();

let location = get(this, 'location');
Expand Down Expand Up @@ -480,7 +484,9 @@ class EmberRouter extends EmberObject {
this._toplevelView = OutletView.create();
this._toplevelView.setOutletState(liveRoutes as GlimmerOutletState);
let instance: any = owner.lookup('-application-instance:main');
instance.didCreateRootView(this._toplevelView);
if (instance) {
instance.didCreateRootView(this._toplevelView);
}
Comment on lines +487 to +489
Copy link
Member

Choose a reason for hiding this comment

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

On a cursory review, this seems somewhat unrelated to the other changes. What circumstances would have forced _setOutlets to be called without an -application-instance:main (IOW why is this change needed)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmmm RenderingTestCase does not have -application-instance:main registered. I guess it's just how ember internal test setup, not having any real impact on user's test. Should we use another base test case class?

Copy link
Member

Choose a reason for hiding this comment

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

No, I think its fine but I'd like to make an explicit test case so we don't accidentally make rendering test case have one and remove the guard. Specifically, I think tests that use a custom resolver (e.g. ember-engines's engineResolverFor) will have this exact issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created a RouterNonApplicationTestCase

} else {
this._toplevelView.setOutletState(liveRoutes as GlimmerOutletState);
}
Expand Down Expand Up @@ -607,6 +613,7 @@ class EmberRouter extends EmberObject {
@method reset
*/
reset() {
this._didSetupRouter = false;
if (this._routerMicrolib) {
this._routerMicrolib.reset();
}
Expand Down
14 changes: 14 additions & 0 deletions packages/@ember/-internals/routing/tests/system/router_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,20 @@ moduleFor(
assert.ok(!router._routerMicrolib);
}

['@test should create a router.js instance after setupRouter'](assert) {
let router = createRouter(undefined, { disableSetup: false });

assert.ok(router._didSetupRouter);
assert.ok(router._routerMicrolib);
}

['@test should return false if setupRouter is called multiple times'](assert) {
let router = createRouter(undefined, { disableSetup: true });

assert.ok(router.setupRouter());
assert.notOk(router.setupRouter());
}

['@test should not reify location until setupRouter is called'](assert) {
let router = createRouter(undefined, { disableSetup: true });
assert.equal(typeof router.location, 'string', 'location is specified as a string');
Expand Down
12 changes: 3 additions & 9 deletions packages/@ember/application/instance.js
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,6 @@ const ApplicationInstance = EngineInstance.extend({
*/
startRouting() {
this.router.startRouting();
this._didSetupRouter = true;
},

/**
Expand All @@ -168,23 +167,18 @@ const ApplicationInstance = EngineInstance.extend({

Because setup should only occur once, multiple calls to `setupRouter`
beyond the first call have no effect.

This is commonly used in order to confirm things that rely on the router
are functioning properly from tests that are primarily rendering related.

For example, from within [ember-qunit](https://github.com/emberjs/ember-qunit)'s
`setupRenderingTest` calling `this.owner.setupRouter()` would allow that
rendering test to confirm that any `<LinkTo></LinkTo>`'s that are rendered
have the correct URL.

@public
*/
setupRouter() {
if (this._didSetupRouter) {
return;
}
this._didSetupRouter = true;

this.router.setupRouter();
},

Expand Down
3 changes: 1 addition & 2 deletions packages/@ember/application/lib/application.js
Original file line number Diff line number Diff line change
Expand Up @@ -1143,7 +1143,7 @@ Application.reopenClass({
});

function commonSetupRegistry(registry) {
registry.register('router:main', Router.extend());
registry.register('router:main', Router);
registry.register('-view-registry:main', {
create() {
return dictionary(null);
Expand All @@ -1167,7 +1167,6 @@ function commonSetupRegistry(registry) {
});

registry.register('service:router', RouterService);
registry.injection('service:router', '_router', 'router:main');
}

function registerLibraries() {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
import { inject as injectService } from '@ember/service';
import { Router, NoneLocation } from '@ember/-internals/routing';
import { get } from '@ember/-internals/metal';
import { run } from '@ember/runloop';
import { Component } from '@ember/-internals/glimmer';
import { RouterNonApplicationTestCase, moduleFor } from 'internal-test-helpers';

moduleFor(
'Router Service - non application test',
class extends RouterNonApplicationTestCase {
constructor() {
super(...arguments);

this.resolver.add('router:main', Router.extend(this.routerOptions));
this.router.map(function () {
this.route('parent', { path: '/' }, function () {
this.route('child');
this.route('sister');
this.route('brother');
});
this.route('dynamic', { path: '/dynamic/:dynamic_id' });
this.route('dynamicWithChild', { path: '/dynamic-with-child/:dynamic_id' }, function () {
this.route('child', { path: '/:child_id' });
});
});
}

get routerOptions() {
return {
location: 'none',
};
}

get router() {
return this.owner.resolveRegistration('router:main');
}

get routerService() {
return this.owner.lookup('service:router');
}

['@test RouterService can be instantiated in non application test'](assert) {
assert.ok(this.routerService);
}

['@test RouterService properties can be accessed with default'](assert) {
assert.expect(5);
assert.equal(this.routerService.get('currentRouteName'), null);
assert.equal(this.routerService.get('currentURL'), null);
assert.ok(this.routerService.get('location') instanceof NoneLocation);
assert.equal(this.routerService.get('rootURL'), '/');
assert.equal(this.routerService.get('currentRoute'), null);
}

['@test RouterService#urlFor returns url'](assert) {
assert.equal(this.routerService.urlFor('parent.child'), '/child');
}

['@test RouterService#transitionTo with basic route'](assert) {
assert.expect(2);

let componentInstance;

this.addTemplate('parent.index', '{{foo-bar}}');

this.addComponent('foo-bar', {
ComponentClass: Component.extend({
routerService: injectService('router'),
init() {
this._super(...arguments);
componentInstance = this;
},
actions: {
transitionToSister() {
get(this, 'routerService').transitionTo('parent.sister');
},
},
}),
template: `foo-bar`,
});

this.render('{{foo-bar}}');

run(function () {
componentInstance.send('transitionToSister');
});

assert.equal(this.routerService.get('currentRouteName'), 'parent.sister');
assert.ok(this.routerService.isActive('parent.sister'));
}

['@test RouterService#recognize recognize returns routeInfo'](assert) {
let routeInfo = this.routerService.recognize('/dynamic-with-child/123/1?a=b');
assert.ok(routeInfo);
let { name, localName, parent, child, params, queryParams, paramNames } = routeInfo;
assert.equal(name, 'dynamicWithChild.child');
assert.equal(localName, 'child');
assert.ok(parent);
assert.equal(parent.name, 'dynamicWithChild');
assert.notOk(child);
assert.deepEqual(params, { child_id: '1' });
assert.deepEqual(queryParams, { a: 'b' });
assert.deepEqual(paramNames, ['child_id']);
}
}
);
1 change: 1 addition & 0 deletions packages/internal-test-helpers/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export { default as AbstractApplicationTestCase } from './lib/test-cases/abstrac
export { default as ApplicationTestCase } from './lib/test-cases/application';
export { default as QueryParamTestCase } from './lib/test-cases/query-param';
export { default as RenderingTestCase } from './lib/test-cases/rendering';
export { default as RouterNonApplicationTestCase } from './lib/test-cases/router-non-application';
export { default as RouterTestCase } from './lib/test-cases/router';
export { default as AutobootApplicationTestCase } from './lib/test-cases/autoboot-application';
export { default as DefaultResolverApplicationTestCase } from './lib/test-cases/default-resolver-application';
Expand Down
Loading