-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Changes from 3 commits
eccf386
156f5d0
72a5592
5d01fd3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
}); | ||
} | ||
}); | ||
} |
There was a problem hiding this comment.
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
isrouteName, ...models, queryParams
, however this is proposingrouteName, ...models, { queryParams }
(destructuringqueryParams
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...There was a problem hiding this comment.
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
andreplaceWith
, 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 thequeryParams
object should be the queryParams itself, not an object having aqueryParams
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 aqueryParams
property that it unpacks. If we go this route we may want to revisit howEmberRouter
is handling extracting the QPs.Thoughts?
There was a problem hiding this comment.
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 aqueryParams
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per the RFC:
There was a problem hiding this comment.
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:
Which is actually incorrect, it should have said:
Or:
(based on the prose just below)
https://github.com/emberjs/rfcs/blob/master/text/0095-router-service.md#new-method-url-generation