Skip to content
This repository has been archived by the owner on Nov 9, 2021. It is now read-only.

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

toovy
Copy link

@toovy toovy commented Jul 10, 2019

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant