aurelia / router

A powerful client-side router.
MIT License
120 stars 115 forks source link

Fix configure router #636

Closed bigopon closed 5 years ago

bigopon commented 5 years ago

Thanks to Fred for tracing it to https://github.com/aurelia/router/commit/a56590e7c8cfa4b7df13871eee6c3d15a1d6c948#diff-82d13b3ecf76478f2ff917591c0dfa2eR87

Add back await to viewModel.configureRouter . This was probably introduced when converting the code back and forth during the big refactoring.

Thanks to @huochunpeng for spotting this.

EisenbergEffect commented 5 years ago

@bigopon I merged. Are there still more changes that need to be made before release?

bigopon commented 5 years ago

@EisenbergEffect i think we're good for now

elmt1 commented 5 years ago

I picked up this update this morning and found that I no longer get links in my navigation panel. I was about 3 months back using 1.6.3 and needed to pick up another fix so deleted my package-lock and rebuilt. That picked up 1.7.1 where it stopped working. Looking at the list of packages that might cause links to not render, I went downgraded just aurelia-router to 1.6.3 and the links worked. Next, I went to 1.7.0 and it continues to work.

Unfortunately, I have some commitments right now but wanted to let you know. I will try to look at it in a few hours. First place I will look is at an auth setting that I use to filter items for navigation with an if.bind.

EisenbergEffect commented 5 years ago

@bigopon Can you take a look?

elmt1 commented 5 years ago

Was easier to confirm than I thought it would be. I just tried deleting the if.bind this from my code and the links render. I had: <a if.bind="!row.settings.auth || row.settings.auth.indexOf(router.options.role) > -1" data-toggle="collapse" data-target="#collapse-target" href.bind="row.href"> <span class="${ row.settings.icon }"></span> ${row.title | terminologyFormat:terminology} </a>

bigopon commented 5 years ago

@elmt1 thanks for the issues. I'll have a look soon

bigopon commented 5 years ago

@elmt1 Could you help give a bit more code so I can reproduce it? From your code above, it seems it's either error in .settings.auth or router.options.role, and those boil down to settings or options not passed to new config object, which I'm uncertain why.

elmt1 commented 5 years ago

@bigopon I would have liked to have given you an example that shows how this fails but I don't know how to force a version in Gist so this is all I could get and is based on a simple WebPack .net core skeleton:



export class App {
    configureRouter(config: RouterConfiguration, router: Router) {
        config.title = "AureliaDotnetTemplate";
        config.options.role = "NotAdmin";
        config.map([{
            route: ["", "home"],
            name: "home",
            settings: { icon: "fa fa-home" },
            moduleId: "../home/home",
            nav: true,
            title: "Home"
        }, {
            route: "counter",
            name: "counter",
            settings: { icon: "fa fa-plus", auth: ["Admin"] },
            moduleId: "../counter/counter",
            nav: true,
            title: "Counter"
        }, {
            route: "fetch-data",
            name: "fetchdata",
                settings: { icon: "fa fa-list", auth: ["NotAdmin"]  },
            moduleId: "../fetchdata/fetchdata",
            nav: true,
            title: "Fetch data"
        }]);
    }
}

<template>
  <nav class="navbar navbar-expand-lg navbar-light bg-light">
    <a class="navbar-brand" href="#/home">Navbar</a>
    <button class="navbar-toggler" type="button" data-toggle="collapse" data-target="#navbarNav" aria-controls="navbarNav" aria-expanded="false" aria-label="Toggle navigation">
      <span class="navbar-toggler-icon"></span>
    </button>

    <div class="navbar-collapse collapse" id="navbarNav">
      <ul class="navbar-nav">
        <li repeat.for="row of router.navigation" class="nav-item ${row.isActive ? 'active' : ''}">
          <a  if.bind="!row.settings.auth || row.settings.auth.indexOf(router.options.role) > -1" href.bind="row.href" class="nav-link">
              <i class="${row.settings.icon}"></i> ${row.title}
          </a>
        </li>
      </ul>
    </div>
  </nav>
</template>```

I hope this helps.
elmt1 commented 5 years ago

Good morning, I started doing some testing and it seems like my html-only navmenu component is the issue. If I back it with a fairly meaningless Navmenu class it works as expected. I am not sure why but maybe it has something to do with load order.

Here is what I added and now it seems to work:

import { Router } from "aurelia-router";
import { autoinject } from "aurelia-framework";

@autoinject
export class Navmenu {
    constructor(private router: Router) { }
}

Hoping this helps.

elmt1 commented 5 years ago

Actually, further testing and that doesn´t seem to be working. I will keep trying different options because I can get this pattern to work in some cases just not in the one I need.

elmt1 commented 5 years ago

This seems like a timing thing. I can make a change in my view and the reloaded page (hot loading) renders fine. If I hit F5 it doesn't render correctly. The router options and navigation look OK at rest. When the menu is built the navigation routes are not available. Things I may be doing differently are injecting the router into the navmenu along with an I18N terminology list. The router appears to have the options I configured, but the routes are not there when the html-only element goes to iterate over the navigation routes.

elmt1 commented 5 years ago

Okay, I have this working now and the workaround was simple but took longer then it should have because I forgot to remove the .html from my require (not the first time I have done that so I should know better). I added a class to my html-only custom element for navigation and injected the router.

Prior to this, I was passing the router and my i18n data to the customer element using bind, which seemed fine and actually worked fine until this change. What I see is that the routes are not on the router soon enough when passed by bind. I think that was synchronous before and now there is a promise building that part.

bigopon commented 5 years ago

@elmt1 can you help elaborate"the routes are not on the router soon enough"? You mean in bind, you couldnt have router.navigation ready?

elmt1 commented 5 years ago

@bigopon, on my main app.html I had a navmenu.html that is an html only custom element with something like:

                    <li repeat.for="row of router.navigation" class="${ row.isActive ? 'link-active' : '' }">
                        <a if.bind="!row.settings.auth || row.settings.auth.indexOf(router.options.role) > -1" data-toggle="collapse" data-target="#collapse-target" href.bind="row.href">
                            <span class="${ row.settings.icon }"></span> ${row.title | terminologyFormat:terminology}
                        </a>
                    </li>

In my app.ts I had something like this:

configureRouter(config: RouterConfiguration, router: Router) {
        config.title = 'my-app';
        router.transformTitle = title => this.terminologyFormatValueConverter.toView(title, this.terminology);
        config.options.role = this.role;

        config.map([
            {
                route: ['', 'page'],
                name: 'page',
                settings: { icon: 'glyphicon glyphicon-page', auth: [ 'Admin', 'AnotherRole' ] },
                moduleId: PLATFORM.moduleName('../page/page'),
                nav: true,
                title: 'page'
            },...

And in the app.html I had something like this:

<navmenu router.bind="router" terminology.bind="terminology"></navmenu>

Using the Aurelia Chrome plugin, I could see the router had everything I would expect but using some $ { } outputs, I was able to see that everything was on the router when the page was being built except for the routes. Toggling between 1.7.0 and 1.7.1 made this clear.

I hope this helps. I have a fairly complete version of this based on the template by @maximbalaganskiy. If it helps I could create a fork of that that demonstrates this issue or share whatever helps.

Thanks!

bigopon commented 5 years ago

@elmt1

Toggling between 1.7.0 and 1.7.1 made this clear.

What was made clear when toggling between those 2 versions? Is this that one version has timing issue and the other doesn't?

And thanks for the input related to timing, I think at least it should help me narrow down where to start chasing this.

elmt1 commented 5 years ago

Yes, I would force 1.7.0 and npm install and it works then force 1.7.1 and npm install and it fails and then going back to 1.7.0 makes it work.