Updates the navigation strategy to handle and expect a resolved strin…

[Roustalski]

…g or object of viewport names with module ids for each key.

Primarily for #289, but also #386

[Roustalski]

I was going to also write some unit tests for this, but I couldn't get the tests to run locally after doing a jspm install. Not sure what that's about.

[EisenbergEffect]

Is there some reason why the custom navigation strategy can't just add the view ports onto the instruction's config? That was the original intent.

[StrahilKazlachev]

When I started using the navigationStrategy I wondered why it didn't work like mapUnknownRoutes, which I find really flexible. Even with this change if you want to set the title you have to modify the instruction NavModel.
@EisenbergEffect You say that the original intent was to add the view ports but I can't find an example in the hub. The only example I found with navigationStrategy does

instruction.config.moduleId = instruction.fragment
instruction.config.href = instruction.fragment

Which does not work if you to return different moduleId on each call(depending on the parameter/s). If you say that augmenting the instruction is the way to go I can add some more to the docs.

[EisenbergEffect]

Can you foresee any issues with just adding a viewPorts property onto the config in the callback?

[Roustalski]

From the app developer's perspective, all they want is for a module to be loaded based on logic within their callback. At least this is the only use-case I've needed so far.

Why not set the appropriate places on the instruction for them based on the resolved string or viewport-ID:module-id object mapping? It's just sugar.

http://aurelia.io/hub.html#/doc/article/aurelia/router/latest/router-configuration/3

    var navStrat = (instruction) => {
      instruction.config.moduleId = instruction.fragment
      instruction.config.href = instruction.fragment
    }

Changes to:

    var navStrat = (instruction) => {
      return Promise.resolve().then(() => instruction.fragment));
    }

If the viewPorts property is added, then it changes to:

    var navStrat = (instruction) => {
      instruction.config.viewPorts['default'] = instruction.fragment;
    }

[EisenbergEffect]

@Roustalski Are you able to sign the CLA? There's a link in the details section of this issue.

[Roustalski]

Alright, I signed the CLA by following the details link and clicking the button, but it still says it hasn't been signed O.o

[EisenbergEffect]

@Roustalski Is there any chance the email associated with your local git isn't the same as Github? We've had this happen a couple of times so we are trying to track down the source. @PWKad Can you chime in on this based on your experience?

[CLAassistant]

All committers have signed the CLA.

[Roustalski]

That was the problem. I have rebased the branch with amended commits with the reset-author option after updating my config to my github name and email

Update: Here is a diff between the rebased commits showing no changes

[EisenbergEffect]

@bryanrsmith See any issues with this?

[davismj]

If a dev returns a falsey value in their nav strat, they might expect it to fall back, but it seems to just break. Changes recommended.

[davismj]

For example, if someone has logic in their nav strat that falls back to a null value, they might expect it to run with the default config, but instead, modules will remain null and this will throw an unhandled error.

I'd recommend updating this to check for a falsey value.

[EisenbergEffect]

@Roustalski Can you update to handle this case?

[Roustalski]

Yea, I'll check this out and update.

[davismj]

@Roustalski Hey Russ, appreciate the time you took on this PR. Did you have a chance to update this PR?

[sorenhoyer]

@StrahilKazlachev I made this repo to replicate another problem I currently have, but I added a console.log(instruction) in app.js on line 24, which - as I understand it - would indicate the exact opposite of what you state here: "The first time navigationStrategy is called navigationInstruction.config does not have viewPorts set".

If you download the repo and run it, you should see that it works just fine - or at least as I would expect it to work.

Maybe I'm just missing your point entirely? :-)

[StrahilKazlachev]

@sorenhoyer There were quite a lot of fixes since my comment.

[Alexander-Taran]

Stale?

[EisenbergEffect]

Defer to @davismj to determine what to do with this.

[davismj]

Since the PR was never updated I can't merge it in. However, I don't have my head in this issue well enough to know if it is important or not. I'd like to come back to it later, but without more interest its low prio.