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

Public Router service MVP #14805

Merged
merged 1 commit into from
Jan 20, 2017
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
3 changes: 2 additions & 1 deletion features.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
"ember-glimmer-allow-backtracking-rerender": null,
"ember-testing-resume-test": null,
"ember-factory-for": true,
"ember-no-double-extend": null
"ember-no-double-extend": null,
"ember-routing-router-service": null
}
}
7 changes: 7 additions & 0 deletions packages/ember-application/lib/system/application.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ import ApplicationInstance from './application-instance';
import { privatize as P } from 'container';
import Engine from './engine';
import { setupApplicationRegistry } from 'ember-glimmer';
import { RouterService } from 'ember-routing';
import { isFeatureEnabled } from 'ember-metal';

let librariesRegistered = false;

Expand Down Expand Up @@ -1037,6 +1039,11 @@ function commonSetupRegistry(registry) {
registry.register('location:none', NoneLocation);

registry.register(P`-bucket-cache:main`, BucketCache);

if (isFeatureEnabled('ember-routing-router-service')) {
registry.register('service:router', RouterService);
registry.injection('service:router', 'router', 'router:main');
}
}

function registerLibraries() {
Expand Down
1 change: 1 addition & 0 deletions packages/ember-routing/lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,5 @@ export { default as Router } from './system/router';
export { default as Route } from './system/route';
export { default as QueryParams } from './system/query_params';
export { default as RoutingService } from './services/routing';
export { default as RouterService } from './services/router';
export { default as BucketCache } from './system/cache';
49 changes: 49 additions & 0 deletions packages/ember-routing/lib/services/router.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
/**
@module ember
@submodule ember-routing
*/

import {
Service,
readOnly
} from 'ember-runtime';
import { get } from 'ember-metal';
import RouterDSL from '../system/dsl';

/**
The Router service is the public API that provides component/view layer
access to the router.

@public
@class RouterService
Copy link
Member

Choose a reason for hiding this comment

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

Should add the @category ember-routing-router-service flag here too (so that while the feature is pending it isn't listed in emberjs.com/api).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@category ember-routing-router-service
*/
const RouterService = Service.extend({
currentRouteName: readOnly('router.currentRouteName'),
currentURL: readOnly('router.currentURL'),
location: readOnly('router.location'),
rootURL: readOnly('router.rootURL'),

/**
Transition the application into another route. The route may
be either a single route or route path:

See [Route.transitionTo](http://emberjs.com/api/classes/Ember.Route.html#method_transitionTo) for more info.

@method transitionTo
@category ember-routing-router-service
@param {String} name 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
containing a mapping of query parameters
@return {Transition} the transition object associated with this
attempted transition
@public
*/
transitionTo() {
this.router.transitionTo(...arguments);
}
});

export default RouterService;
8 changes: 8 additions & 0 deletions packages/ember-routing/lib/system/router.js
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,10 @@ const EmberRouter = EmberObject.extend(Evented, {
init() {
this._super(...arguments);

this.currentURL = null;
this.currentRouteName = null;
this.currentPath = null;

this._qpCache = new EmptyObject();
this._resetQueuedQueryParameterChanges();
this._handledErrors = dictionary(null);
Expand Down Expand Up @@ -629,6 +633,7 @@ const EmberRouter = EmberObject.extend(Evented, {

let doUpdateURL = function() {
location.setURL(lastURL);
set(emberRouter, 'currentURL', lastURL);
Copy link
Member

Choose a reason for hiding this comment

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

I think we should actually do this in the router.updateURL and router.replaceURL methods directly above the run.once(doUpdateURL) bit...

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 can do that. My thought was I wanted to keep both set calls in the same place, but these methods are so closely related that it's find either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, so it turns out that setting this after a scheduled call is what's fixing this issue. So, it looks like I have to leave it where it is.

};

router.updateURL = function(path) {
Expand All @@ -639,6 +644,7 @@ const EmberRouter = EmberObject.extend(Evented, {
if (location.replaceURL) {
let doReplaceURL = function() {
location.replaceURL(lastURL);
set(emberRouter, 'currentURL', lastURL);
};

router.replaceURL = function(path) {
Expand Down Expand Up @@ -1291,9 +1297,11 @@ function updatePaths(router) {

let path = EmberRouter._routePath(infos);
let currentRouteName = infos[infos.length - 1].name;
let currentURL = router.get('location').getURL();

set(router, 'currentPath', path);
set(router, 'currentRouteName', currentRouteName);
set(router, 'currentURL', currentURL);
Copy link
Member

Choose a reason for hiding this comment

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

I would think this change is not needed since we are setting router.currentURL above in the changes to _setupRouter.

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 thought this too, initially. It turns out that there's a different codepath whether you're visiting vs. transitioning, and we need this update to occur in both places. I think this surfaces part of @krisselden's concerns, where we want to take advantage, while we have the engine out on the floor, to refactor this to make it cleaner and more consistent.


let appController = getOwner(router).lookup('controller:application');

Expand Down
Loading