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

Phase 3 of public Router Service - adding urlFor #14979

Merged
merged 4 commits into from
Mar 8, 2017
Merged
Show file tree
Hide file tree
Changes from 3 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
24 changes: 20 additions & 4 deletions packages/ember-routing/lib/services/router.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ const RouterService = Service.extend({

@method transitionTo
@category ember-routing-router-service
@param {String} name the name of the route or a URL
@param {String} routeNameOrUrl the name of the route or a URL
@param {...Object} models the model(s) or identifier(s) to be used while
transitioning to the route.
@param {Object} [options] optional hash with a queryParams property
Expand All @@ -41,7 +41,7 @@ const RouterService = Service.extend({
attempted transition
@public
*/
transitionTo() {
transitionTo(/* routeNameOrUrl, ...models, options */) {
return this.router.transitionTo(...arguments);
},

Expand All @@ -53,7 +53,7 @@ const RouterService = Service.extend({

@method replaceWith
@category ember-routing-router-service
@param {String} name the name of the route or a URL
@param {String} routeNameOrUrl the name of the route or a URL
@param {...Object} models the model(s) or identifier(s) to be used while
transitioning to the route.
@param {Object} [options] optional hash with a queryParams property
Expand All @@ -62,8 +62,24 @@ const RouterService = Service.extend({
attempted transition
@public
*/
replaceWith() {
replaceWith(/* routeNameOrUrl, ...models, options */) {
return this.router.replaceWith(...arguments);
},

/**
Generate a URL based on the supplied route name.

@method urlFor
@param {String} routeName the name of the route
@param {...Object} models the model(s) or identifier(s) to be used while
transitioning to the route.
@param {Object} [options] optional hash with a queryParams property
Copy link
Member

@rwjblue rwjblue Mar 3, 2017

Choose a reason for hiding this comment

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

The RFC suggests that the argument signature for urlFor is routeName, ...models, queryParams, however this is proposing routeName, ...models, { queryParams } (destructuring queryParams off of the last "options" argument). See RFC details here.

Why are we diverging? Did you discover something during implementation that suggests we should have these diverge from the RFC? I'm guessing that its so that we have parity with .transitionTo and .replaceWith, but I wanted to confirm...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My interpretation was that we should mimic the signatures of transitionTo and replaceWith, as it's preferable to have a consistent API across all public methods. That said, I think what you said here is totally legit, in that the queryParams object should be the queryParams itself, not an object having a queryParams property that is the actual queryParams.

I'm going to revisit all of them for consistency, but I know that EmberRouter expects an object that has a queryParams property that it unpacks. If we go this route we may want to revisit how EmberRouter is handling extracting the QPs.

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think it makes sense to use the same interface as .transitionTo / .replaceWith (which both have options with a queryParams property on them). We can get @ef4 and others to confirm just before landing, but I think it makes sense to stay consistent for now.

If we do decide to change the API, they should all be changed, which is definitely unrelated to this PR...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As per the RFC:

This takes the same arguments as transitionTo

Copy link
Member

Choose a reason for hiding this comment

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

HAHA! Derp. I just was focusing on the code snippet:

urlFor(routeName, ...models, queryParams)

Which is actually incorrect, it should have said:

urlFor(routeName, ...models, { queryParams })

Or:

urlFor(routeName, ...models, options)

(based on the prose just below)

https://github.com/emberjs/rfcs/blob/master/text/0095-router-service.md#new-method-url-generation

containing a mapping of query parameters
@return {String} the string representing the generated URL
@public
*/
urlFor(/* routeName, ...models, options */) {
return this.router.generate(...arguments);
}
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,24 @@ if (isFeatureEnabled('ember-routing-router-service')) {
});
}

['@test RouterService#replaceWith with basic route using URLs replaces location'](assert) {
assert.expect(1);

return this.visit('/')
.then(() => {
return this.routerService.transitionTo('/child');
})
.then(() => {
return this.routerService.transitionTo('/sister');
})
.then(() => {
return this.routerService.replaceWith('/brother');
})
.then(() => {
assert.deepEqual(this.state, ['/', '/child', '/brother']);
});
}

['@test RouterService#replaceWith transitioning back to previously visited route replaces location'](assert) {
assert.expect(1);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,38 @@ if (isFeatureEnabled('ember-routing-router-service')) {
});
}

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

let componentInstance;

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

this.registerComponent('foo-bar', {
ComponentClass: Component.extend({
routerService: inject.service('router'),
init() {
this._super();
componentInstance = this;
},
actions: {
transitionToSister() {
get(this, 'routerService').transitionTo('/sister');
}
}
}),
template: `foo-bar`
});

return this.visit('/').then(() => {
run(function() {
componentInstance.send('transitionToSister');
});

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

['@test RouterService#transitionTo with dynamic segment'](assert) {
assert.expect(3);

Expand Down
162 changes: 162 additions & 0 deletions packages/ember/tests/routing/router_service_test/urlFor_test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,162 @@
import {
Controller,
inject,
String
} from 'ember-runtime';
import { Component } from 'ember-glimmer';
import { Route, NoneLocation } from 'ember-routing';
import {
get,
set
} from 'ember-metal';
import {
RouterTestCase,
moduleFor
} from 'internal-test-helpers';

import { isFeatureEnabled } from 'ember-metal';

function defineController(app, name) {
let controllerName = `${String.capitalize(name)}Controller`;

Object.defineProperty(app, controllerName, {
get() {
throw new Error(`Generating a URL should not require instantiation of a ${controllerName}.`);
}
});
}

function buildQueryParams(queryParams) {
return {
queryParams
};
}

if (isFeatureEnabled('ember-routing-router-service')) {
moduleFor('Router Service - urlFor', class extends RouterTestCase {
constructor() {
Copy link
Member

Choose a reason for hiding this comment

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

This constructor isn't really needed here, can remove..

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 had been doing some custom work in this constructor, but removed it. The constructor was just left in place. Will remove!

super();

['dynamic', 'child']
.forEach((name) => { defineController(this.application, name); });
}

['@test RouterService#urlFor returns URL for simple route'](assert) {
assert.expect(1);

return this.visit('/').then(() => {
let expectedURL = this.routerService.urlFor('parent.child');

assert.equal('/child', expectedURL);
});
}

['@test RouterService#urlFor returns URL for simple route with dynamic segments'](assert) {
assert.expect(1);

let dynamicModel = { id: 1, contents: 'much dynamicism' };

return this.visit('/').then(() => {
let expectedURL = this.routerService.urlFor('dynamic', dynamicModel);

assert.equal('/dynamic/1', expectedURL);
});
}

['@test RouterService#urlFor returns URL for simple route with basic query params'](assert) {
assert.expect(1);

let queryParams = buildQueryParams({ foo: 'bar' });

return this.visit('/').then(() => {
let expectedURL = this.routerService.urlFor('parent.child', queryParams);

assert.equal('/child?foo=bar', expectedURL);
});
}

['@test RouterService#urlFor returns URL for simple route with array as query params'](assert) {
assert.expect(1);

let queryParams = buildQueryParams({ selectedItems: ['a', 'b', 'c'] });

return this.visit('/').then(() => {
let expectedURL = this.routerService.urlFor('parent.child', queryParams);

assert.equal('/child?selectedItems[]=a&selectedItems[]=b&selectedItems[]=c', expectedURL);
});
}

['@test RouterService#urlFor returns URL for simple route with null query params'](assert) {
assert.expect(1);

let queryParams = buildQueryParams({ foo: null });

return this.visit('/').then(() => {
let expectedURL = this.routerService.urlFor('parent.child', queryParams);

assert.equal('/child', expectedURL);
});
}

['@test RouterService#urlFor returns URL for simple route with undefined query params'](assert) {
assert.expect(1);

let queryParams = buildQueryParams({ foo: undefined });

return this.visit('/').then(() => {
let expectedURL = this.routerService.urlFor('parent.child', queryParams);

assert.equal('/child', expectedURL);
});
}

['@test RouterService#urlFor returns URL for simple route with dynamic segments and basic query params'](assert) {
assert.expect(1);

let queryParams = buildQueryParams({ foo: 'bar' });

return this.visit('/').then(() => {
let expectedURL = this.routerService.urlFor('dynamic', { id: 1 }, queryParams);

assert.equal('/dynamic/1?foo=bar', expectedURL);
});
}

['@test RouterService#urlFor returns URL for simple route with dynamic segments and array as query params'](assert) {
assert.expect(1);

let queryParams = buildQueryParams({ selectedItems: ['a', 'b', 'c'] });

return this.visit('/').then(() => {
let expectedURL = this.routerService.urlFor('dynamic', { id: 1 }, queryParams);

assert.equal('/dynamic/1?selectedItems[]=a&selectedItems[]=b&selectedItems[]=c', expectedURL);
});
}

['@test RouterService#urlFor returns URL for simple route with dynamic segments and null query params'](assert) {
assert.expect(1);

let queryParams = buildQueryParams({ foo: null });

return this.visit('/').then(() => {
let expectedURL = this.routerService.urlFor('dynamic', { id: 1 }, queryParams);

assert.equal('/dynamic/1', expectedURL);
});
}

['@test RouterService#urlFor returns URL for simple route with dynamic segments and undefined query params'](assert) {
assert.expect(1);

let queryParams = buildQueryParams({ foo: undefined });

return this.visit('/').then(() => {
let expectedURL = this.routerService.urlFor('dynamic', { id: 1 }, queryParams);

assert.equal('/dynamic/1', expectedURL);
});
}
});
}
2 changes: 1 addition & 1 deletion packages/internal-test-helpers/lib/test-cases/router.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ export default class RouterTestCase extends ApplicationTestCase {
this.route('sister');
this.route('brother');
});
this.route('dynamic', { path: '/dynamic/:post_id' });
this.route('dynamic', { path: '/dynamic/:dynamic_id' });
});
}

Expand Down