poteto / ember-crumbly

Declarative breadcrumb navigation for Ember apps
MIT License
167 stars 78 forks source link

🐞 fixed an issue with first route segments index-route lookup #144

Open toovy opened 5 years ago

toovy commented 5 years ago

We noticed a strange behaviour in our real-life app that only occurs when setting the breadCrumb property in afterModel.

In our example we use ember-intl to set the breadCrumbs title in afterModel. When using administration/imagesizes we could see a random behaviour. We could see that the route lookup either delivered a result or not. So randomly the title of either administration or administration.index was used for the first segment. The following code lines could be identified to cause the issue:

return (this._lookupRoute(path)) ? path : name;

This call looked up the route in the container:

_lookupLocalRoute(routeName) {
   return getOwner(this).lookup(`route:${routeName}`);
 },

As mentioned this._lookupRoute(path) randomly returned the route or undefined. So either the name or path was returned with different effects.

When the name was returned, i.e. administration, then our translations worked fine, as afterModel was called on the administration route.

But when the path was returned our translations did not work, the default title was used. I found out why: the afterModel hook of the route administration.index is never called on sub-routes of administration (e.g. administration/imagesizes).

I think the behaviour is correct, as administration.index might be instantiated in the container, but as long as the route is not "active", no hooks are called.

I assume that the idea of getOwner(this).lookup(route:${routeName}) is to check if the route is active. So I think that the correct logic would be to check if administration.index is really "active" instead of just looking it up in the container. Using the router service this is possible:

const isIndexRouteActive = this.get("routerService").isActive(path);
return (isIndexRouteActive) ? path : name;

In our application this works really fine. I noticed that three tests would fail after the change:

not ok 15 Chrome 75.0 - [252 ms] - Acceptance | ember-crumbly integration test: bread-crumbs component accepts a block
  actual: >
      Derek Zoolander's Zoo for Animals Who Can't Read Good and Want to Do Other Stuff Good Too
  expected: >
      Animals at the Zoo
  stack: >

not ok 17 Chrome 75.0 - [184 ms] - Acceptance | ember-crumbly integration test: absence of reverse option renders breadcrumb right to left
  actual: >
      I am Foo,I am Bar,I am Baz
  expected: >
      I am Foo Index,I am Bar,I am Baz

not ok 18 Chrome 75.0 - [184 ms] - Acceptance | ember-crumbly integration test: reverse option = TRUE renders breadcrumb from left to right
  actual: >
      I am Baz,I am Bar,I am Foo
  expected: >
      I am Baz,I am Bar,I am Foo Index

After looking through the tests I assume that the tests are incorrectly assume that the first segments index route is loaded. I guess that it should be the first segments route. E.g. in test No. 15 the route is /animal/quadruped/cow/show, so it should load the breadCrumb title of animal, not animal.index. But the test checked if the breadCrumb title of animal.index was loaded. That's why I guess the tests are incorrect, so I fixed them.

I hope that everything makes sense and what I changed was correct. This fix will change the behaviour of ember-crumbly slightly and might affect existing installations. But it will be more reliable in the future.