Phase 3 of public Router Service - adding urlFor #14979

scalvert · 2017-03-03T03:50:01Z

This PR represents the second phase of the public router service described in this RFC.

It extends the API with:

urlFor

Status: Work in progress

Reviewers: @locks @rwjblue @ef4

Changes:

Added urlFor method in public router service
Added tests for urlFor
Added some extra tests for transitionTo and replaceWith that cover URLs as the first parameter

How to test drive this PR:

yarn start
Open up 2 tabs pointing to http://localhost:4200/tests/index.html, one with Enable Opt Features checked, ensure tests pass

chadhietala · 2017-03-03T04:38:40Z

Looking good. What is left? More tests? Really excited about this!

rwjblue

Looking good!

I think we need test cases for the following scenarios:

router.urlFor('something', { selectedItems: ['a', 'b', 'c'] }) (a query param that is an array)
router.urlFor('something', { stuff: null })
router.urlFor('something', { stuff: undefined })
router.urlFor('something', { stuff: 'foo' }) where something route defines a custom serializeQueryParam method
Tests defining a controller for the target, where the controller throws an error in init. The idea here is to ensure that we are not forcing controller instances to be created when generating the URL (which is a significant part of the performance issues surrounding {{link-to}}).

rwjblue · 2017-03-03T14:20:39Z

packages/ember-routing/lib/system/router.js

-  generate() {
-    let url = this._routerMicrolib.generate(...arguments);
+  generate(...args) {
+    let url = this._routerMicrolib.generate(...args);


Why do we need this change? As you can see here the transpiled output is the same before and after.

Seems fine to make the change, but I was just curious what motivated it...

I added this while I was fiddling with the params to ensure they were passed through to _routerMicrolib.generate in the expected format. I realized that this method handles extracting the params correctly already, so this is just a remnant of that. Will revert for sure!

rwjblue · 2017-03-03T14:22:13Z

packages/ember/tests/routing/router_service_test/urlFor_test.js

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


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

I had been doing some custom work in this constructor, but removed it. The constructor was just left in place. Will remove!

rwjblue · 2017-03-03T14:28:49Z

packages/ember-routing/lib/services/router.js

+     @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


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...

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?

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...

As per the RFC:

This takes the same arguments as transitionTo

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

rwjblue · 2017-03-03T15:13:24Z

Tests defining a controller for the target, where the controller throws an error in init. The idea here is to ensure that we are not forcing controller instances to be created when generating the URL (which is a significant part of the performance issues surrounding {{link-to}}).

This JSBin Demo demonstrates that a bit more work will be needed to satisfy this constraint.

Currently, when routerMicrolib invokes its getHandler function (defined here), we look up the route, then eagerly invoke route._populateQPMeta() (here).

rwjblue · 2017-03-03T15:53:33Z

Currently, when routerMicrolib invokes its getHandler function (defined here), we look up the route, then eagerly invoke route._populateQPMeta() (here).

As I was digging into this, I was able to chart a path forward. Please review #14980, which should resolve the specific entanglement I mentioned above...

scalvert · 2017-03-03T16:57:41Z

@chadhietala more tests, working out specifics of performance characteristics (@rwjblue already found issues with eager instantiation of controllers), etc.

scalvert · 2017-03-03T17:06:44Z

@rwjblue thanks for all the investigation! I totally appreciate it.

The controller.init -> error test case was one that we'd discussed before, and is definitely one I want to add. I'm going to start listing all the test cases in a todo in the description of this PR, so we can ensure we're covering all the permutations.

I've reviewed #14980 and dropped a comment on it.

chadhietala · 2017-03-03T17:20:01Z

Since this is one of the places where we need to improve on performance I think it might be useful to use a spy on methods we know we should not enter or entered a well known amount of times. In the long term we need to get Heimdall instrumentation in here, but I think asserting call counts in the tests can give us some confidence.

…ollers are looked up

scalvert · 2017-03-05T19:46:50Z

@rwjblue

Looks like #14980 correctly resolved urlFor causing lookups on controllers. I verified this by adding the getters to the app as per your JSBin, ensuring the tests failed, and subsequently merging in the upstream changes. After the merge the tests now pass.

⭐️

scalvert · 2017-03-06T17:44:21Z

@rwjblue added more tests for permutations of calls. What's left? I think you've addressed the expensive calls in URL generation. Is there any other work that needs to happen there? Are there any other checks we can put in place within tests to ensure we don't regress down the non-performant path?

rwjblue

Additional tests needed:

generating path via routerService.urlFor(...stuff...) then calling routerService.transitionTo(pathGenerated) ends up on the same path as urlFor generated
- no dynamic segments (models) or query params
- with dynamic segments (models) no query params
- no dynamic segments (models) with query params
- with dynamic segments (models) and query params
Still need tests for paths generated for routes including a custom serializeQueryParam

scalvert · 2017-03-08T01:43:17Z

Good ideas on the tests! Will work on adding those.

The last one where we're testing paths generated for routes including a custom serializeQueryParam - shouldn't we wait until we fix the QP serialization, which we discussed would be a fast follow of this work?

rwjblue · 2017-03-08T01:45:12Z

Yep, sounds good! Also, those serializeQueryParam tests and implementation work can close #11288.

rwjblue · 2017-03-08T12:29:12Z

Thank you @scalvert!

stefanpenner approved these changes Mar 3, 2017

View reviewed changes

rwjblue requested changes Mar 3, 2017

View reviewed changes

scalvert added 2 commits March 5, 2017 11:38

Router Service Phase 3 - urlFor

eccf386

Adding controllers to ensure they throw when calling urlFor and contr…

156f5d0

…ollers are looked up

scalvert force-pushed the router-service branch from 1add58d to 156f5d0 Compare March 5, 2017 19:41

Adding more tests for permutations of urlFor calls

72a5592

rwjblue reviewed Mar 7, 2017

View reviewed changes

Adding more tests for ensuring we route to generated URL

5d01fd3

rwjblue approved these changes Mar 8, 2017

View reviewed changes

rwjblue merged commit 431be5c into emberjs:master Mar 8, 2017

acorncom mentioned this pull request Mar 8, 2017

Unable to override serializeQueryParam within route #11288

Open

scalvert deleted the router-service branch March 9, 2017 03:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Phase 3 of public Router Service - adding urlFor #14979

Phase 3 of public Router Service - adding urlFor #14979

scalvert commented Mar 3, 2017 •

edited

Loading

chadhietala commented Mar 3, 2017

rwjblue left a comment

rwjblue Mar 3, 2017

scalvert Mar 3, 2017

rwjblue Mar 3, 2017

scalvert Mar 3, 2017

rwjblue Mar 3, 2017 •

edited

Loading

scalvert Mar 3, 2017

rwjblue Mar 3, 2017

scalvert Mar 3, 2017

rwjblue Mar 3, 2017

rwjblue commented Mar 3, 2017

rwjblue commented Mar 3, 2017

scalvert commented Mar 3, 2017

scalvert commented Mar 3, 2017

chadhietala commented Mar 3, 2017 •

edited

Loading

scalvert commented Mar 5, 2017 •

edited

Loading

scalvert commented Mar 6, 2017 •

edited

Loading

rwjblue left a comment

scalvert commented Mar 8, 2017

rwjblue commented Mar 8, 2017

rwjblue commented Mar 8, 2017

Phase 3 of public Router Service - adding urlFor #14979

Phase 3 of public Router Service - adding urlFor #14979

Conversation

scalvert commented Mar 3, 2017 • edited Loading

chadhietala commented Mar 3, 2017

rwjblue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwjblue Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwjblue commented Mar 3, 2017

rwjblue commented Mar 3, 2017

scalvert commented Mar 3, 2017

scalvert commented Mar 3, 2017

chadhietala commented Mar 3, 2017 • edited Loading

scalvert commented Mar 5, 2017 • edited Loading

scalvert commented Mar 6, 2017 • edited Loading

rwjblue left a comment

Choose a reason for hiding this comment

scalvert commented Mar 8, 2017

rwjblue commented Mar 8, 2017

rwjblue commented Mar 8, 2017

scalvert commented Mar 3, 2017 •

edited

Loading

rwjblue Mar 3, 2017 •

edited

Loading

chadhietala commented Mar 3, 2017 •

edited

Loading

scalvert commented Mar 5, 2017 •

edited

Loading

scalvert commented Mar 6, 2017 •

edited

Loading