Skip to content

Commit

Permalink
feat(router): add rest method
Browse files Browse the repository at this point in the history
This partially addresses aurelia/framework#212
  • Loading branch information
EisenbergEffect committed Jan 5, 2016
1 parent 9beaaaa commit dbce60c
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 14 deletions.
13 changes: 10 additions & 3 deletions src/app-router.js
Expand Up @@ -15,13 +15,20 @@ const logger = LogManager.getLogger('app-router');
export class AppRouter extends Router {
static inject() { return [Container, History, PipelineProvider, EventAggregator]; }

_queue = [];

constructor(container: Container, history: History, pipelineProvider: PipelineProvider, events: EventAggregator) {
super(container, history);
super(container, history); //Note the super will call reset internally.
this.pipelineProvider = pipelineProvider;
this.events = events;
}

/**
* Fully resets the router's internal state. Primarily used internally by the framework when multiple calls to setRoot are made.
* Use with caution (actually, avoid using this). Do not use this to simply change your navigation model.
*/
reset() {
super.reset();
this.maxInstructionCount = 10;
this._queue = [];

This comment has been minimized.

Copy link
@davismj

davismj Jan 5, 2016

Member

@EisenbergEffect Would assigning a new array to queue be a problem? If so, perhaps this._queue.splice(0,Infinity) would be more robust?

This comment has been minimized.

Copy link
@EisenbergEffect

EisenbergEffect Jan 5, 2016

Author Contributor

Can you elaborate on why it might be a problem?

This comment has been minimized.

Copy link
@davismj

davismj Jan 5, 2016

Member

If anything has assigned queue referrentially (binding.value = router._queue), this reference is to the array you've replaced on line 31. Not only is it a memory leak, as the old queue array will remain in memory, but it may also produce unwanted behavior.

this._queue.splice(0,Infinity) simply clears the array without instantiating a new array. There is no potential for memory leak or unwanted side effects.

This comment has been minimized.

Copy link
@EisenbergEffect

EisenbergEffect Jan 5, 2016

Author Contributor

I think the best way would be this._queue.length = 0 based on my reading. Would you mind submitting a PR to make this change? I'll get it merged ASAP.

This comment has been minimized.

Copy link
@davismj

davismj Jan 5, 2016

Member

Okay, I'll do that now.

}

/**
Expand Down
35 changes: 24 additions & 11 deletions src/router.js
Expand Up @@ -19,28 +19,28 @@ import {RouteConfig} from './interfaces';
export class Router {
container: Container;
history: History;
viewPorts: Object = {};
routes: RouteConfig[] = [];
viewPorts: Object;
routes: RouteConfig[];

/**
* The [[Router]]'s current base URL, typically based on the [[Router.currentInstruction]].
*/
baseUrl: string = '';
baseUrl: string;

/**
* True if the [[Router]] has been configured.
*/
isConfigured: boolean = false;
isConfigured: boolean;

/**
* True if the [[Router]] is currently processing a navigation.
*/
isNavigating: boolean = false;
isNavigating: boolean;

/**
* The navigation models for routes that specified [[RouteConfig.nav]].
*/
navigation: NavModel[] = [];
navigation: NavModel[];

/**
* The currently active navigation instruction.
Expand All @@ -50,11 +50,7 @@ export class Router {
/**
* The parent router, or null if this instance is not a child router.
*/
parent: Router;

_fallbackOrder: number = 100;
_recognizer: RouteRecognizer = new RouteRecognizer();
_childRecognizer: RouteRecognizer = new RouteRecognizer();
parent: Router = null;

/**
* @param container The [[Container]] to use when child routers.
Expand All @@ -63,7 +59,24 @@ export class Router {
constructor(container: Container, history: History) {
this.container = container;
this.history = history;
this.reset();
}

/**
* Fully resets the router's internal state. Primarily used internally by the framework when multiple calls to setRoot are made.
* Use with caution (actually, avoid using this). Do not use this to simply change your navigation model.
*/
reset() {
this.viewPorts = {};
this.routes = [];
this.baseUrl = '';
this.isConfigured = false;
this.isNavigating = false;
this.navigation = [];
this.currentInstruction = null;

This comment has been minimized.

Copy link
@bryanrsmith

bryanrsmith Jan 5, 2016

Contributor

I think this will cause us to leak all currently active views. We may need to leave currentInstruction and possibly viewPorts intact.

This comment has been minimized.

Copy link
@EisenbergEffect

EisenbergEffect Jan 5, 2016

Author Contributor

Hmmm. That's a good point. This is probably an ok temporary fix for the use cases involved. Lets think about this and see how we can improve it soon.

This comment has been minimized.

Copy link
@davismj

davismj Jan 5, 2016

Member

Yes, as noted here (aurelia/framework#212 (comment)), one and arguably the best strategy would be a full deactivation, including of any active views. This was a huge setback for Durandal as active views on deactivated roots were left floating around with unwanted behavior.

This comment has been minimized.

Copy link
@EisenbergEffect

EisenbergEffect Jan 5, 2016

Author Contributor

We may be able to do this in the future. The setRoot is detecting the root router presence, so it could do something like that. Bear in mind, there is no concept of deactivating something without navigating somewhere. So, there's no api to actually do this right now. Also, there's no way to "force" deactivation, which would be required in this case...otherwise the call to setRoot could fail if a canDeactivate method returned false.

This comment has been minimized.

Copy link
@davismj

davismj Jan 5, 2016

Member

One of my original thoughts was to have a hidden router that handles setting roots. Each root is a dynamic route on the root router. This would handle all of the above, I think.

This comment has been minimized.

Copy link
@EisenbergEffect

EisenbergEffect Jan 5, 2016

Author Contributor

But we don't want to create a dependency between the framework and the router. I think what you are describing could be done today by creating a root component with that same router explicitly defined.

What we can do is allow the framework to detect a router's presence and call some of its apis, if it implements them. This could allow the proper cleanup.

this._fallbackOrder = 100;
this._recognizer = new RouteRecognizer();
this._childRecognizer = new RouteRecognizer();
this._configuredPromise = new Promise(resolve => {
this._resolveConfiguredPromise = resolve;
});
Expand Down

0 comments on commit dbce60c

Please sign in to comment.