UI: Refactor breadcrumbs

[DingoEatingFuzz]

The primary motivation for this refactor was to reduce a bunch of copy/pasting.

The existing implementation made sense in the early days when breadcrumbs were unknown and the full extent of how they would be used was being figured out. Now that the design has settled and there are many pages in the UI, a refactor is due.

High-level Overview

1. Breadcrumbs are now defined in routes instead of in templates and/or controllers.
2. A new breadcrumbs service computes breadcrumbs based on the current route. It watches for changes to currentRouteName and currentURL to know when the page has changed. When the page changes, it looks up all the routes in the current hierarchy and concatenates crumbs. This is similar to how to ember-crumbly works, but different enough that it didn't make sense to try and make using ember-crumbly work.
3. A new app-breadcrumbs component gets the breadcrumbs information from the breadcrumbs service and renders them. Breadcrumbs can be objects with a label and link-to args or a PromiseProxyObject that resolves with the appropriate object.
4. This means that breadcrumbs are implemented the same way on every page so the template code can be removed from leaf templates and moved up the view tree to common templates like jobs.hbs.

The diff only has more insertions than deletions because test coverage went up. Full controllers were deleted since they were only used to manage breadcrumbs.

Unusual Features

1. A route can have many breadcrumbs. This is used in a couple places. One is child jobs. The route for a child job isn't nested under a parent job but the breadcrumbs trail should still represent that parent job. The other is allocations. Allocations get their own route hierarchy, matching the API, but extend job breadcrumbs since in practice allocations are an artifact of jobs.
2. link-to args aren't determined strictly by the route. This is partly because of the previous unusual feature and also because of the namespace query param.

[johncowen]

Hey @DingoEatingFuzz

I've had a brief look over this and have a few initial thoughts. The main thing is right now I've tried this branch and I'm not sure if I'm seeing things correctly.

When I first checked the branch and tried the breadcrumbs I kept getting a grey error page when clicking the penultimate breadcrumb. So I checked out master again, and strangely kept getting a similar grey error page when doing the same. I didn't think to screengrab this at the time, but it doesn't look like this is happening now, must've been something I'd done

I rechecked out this branch, deleted the tmp folder, and tried again. Now I don't get the error message but I get inconsistent 'breadcrumbing'.

First time round I got this:

I checked this with the public nomad demo site and I'm pretty sure I should see more breadcrumbs here?

I restarted the app and tried again, and this time I get this:

I'm pretty sure that it still should give me more breadcrumbs, but even more strange is that its giving my 1 extra than before?

I don't know maybe I've misunderstood what I should be seeing. Feel free to ping me when you are about and we can check over together if it helps.

Apart from this I think its definitely a good move to refactor this a little. I found I'd get lost quite easily when working on this a few months ago. I've had a quick scan over your approach here, before saying the following I'd caveat it with 'different strokes for different folks' so please take it as different thinking rather than anything else.

• Have you considered passing the breadcrumb data into the {{app-breadcrumbs}} component as an attribute on the component itself (i.e. data down) rather than the component essentially doing the work of pulling them from the route? IMHO right now its quite tightly coupled to the route and the control is wrapped up inside the component rather than you being able to control it from the outside. Passing the data through on the attributes means the component doesn't need to know where to get the breadcrumbs from and is only concerned with rendering them. I can see you are probably trying to make it work as 'automatically' as possible here like crumbly, but I'd be cautious of the coupling.

• breadcrumbs-utils looks like its some sort of generic utility module, yet looking closer it only seems to be concerned with breadcrumbs for jobs specifically? Maybe put it in a further subfolder inside utils like utils/job or maybe rename it?

• I like that you've moved the breadcrumbs models out of the Controllers and you've brought Routes into the mix. I think you know my thoughts on this, but what do you think of the idea of the routes only being responsible for collecting data from a model layer and passing them through to the view layer? Following this thinking you could potentially have a breadcrumbsRepo service that was purely responsible for providing a way for the routes to collect the breadcrumbs models and pass them through to the view ready to by picked up by your component {{app-breadcrumbs breadcrumbs=breadcrumbs }}. Your breadcrumbsRepo service could have methods like findByAllocationId(id), findByTaskGroupId(id) etc etc. Right now you are building up your breadcrumbs manually, and this could all then be done in the same service instead of in all your routes, and potentially moving forwards if you want to create them some other way, this could all be dealt with in the same service or 'repository'.

I'll have another look a bit later on and see if I can figure why I'm getting inconsistencies, it might be something I'm doing wrong, but definitely shout me later if you want and we can talk through it more.

Cheers

[DingoEatingFuzz]

I don't know maybe I've misunderstood what I should be seeing. Feel free to ping me when you are about and we can check over together if it helps.

Those screenshots are definitely off. I just ran through everything again locally and everything looks right. I also proxied to the demo cluster (ember serve --proxy=https://demo.nomadproject.io) and everything checked out there too.

before saying the following I'd caveat it with 'different strokes for different folks' so please take it as different thinking rather than anything else.

Ember is a conventions over configuration framework, so ideally the amount of "different strokes for different folks" decision-making is minimal.

Have you considered passing the breadcrumb data into the {{app-breadcrumbs}} component as an attribute on the component itself (i.e. data down) rather than the component essentially doing the work of pulling them from the route?

Yes. Right now the component can accept a breadcrumbs attribute to make it a little more reusable. However, in practice this doesn't happen. The whole point of this work was to get repetitive breadcrumbs code out of templates and into a higher-level place. An alternative that gets part of the way there would be to remove the dependency app-breadcrumbs has on the breadcrumbs service. The downside to this is a bunch of repetitive service injections in common controllers.

IMHO right now its quite tightly coupled to the route

I disagree. The component is coupled to a service and that service gets breadcrumbs from Routes, but the component doesn't know that. If the service were to generate breadcrumbs in some other way (as seen in integration tests) the component works fine. That's strong evidence that there isn't a tight coupling to routes.

the control is wrapped up inside the component rather than you being able to control it from the outside.

I'm assuming by control here you mean the content? In that case, as mentioned, breadcrumbs can be overridden from the outside.

I can see you are probably trying to make it work as 'automatically' as possible here like crumbly

Yep, exactly this.

I'd be cautious of the coupling.

Definitely something worth being cautious of. I think the delineation of a route being responsible for its own breadcrumbs, a service responsible for assembling the full breadcrumb trail, and a component responsible for rendering the trail is ideal separation of concerns. This model is supported by both crumbly and TFE.

breadcrumbs-utils looks like its some sort of generic utility module, yet looking closer it only seems to be concerned with breadcrumbs for jobs specifically? Maybe put it in a further subfolder inside utils like utils/job or maybe rename it?

Right now it only has job related code, but it could have varying stuff in the future. It almost had task-group related code, but that code turned out to not be as reusable as I had hoped.

I think you know my thoughts on this, but what do you think of the idea of the routes only being responsible for collecting data from a model layer and passing them through to the view layer?

In your mind, what is the "view layer"? At the very least, the route would have to pass it to the controller to pass it to a template. This sounds like a lot of repetitive code.

Following this thinking you could potentially have a breadcrumbsRepo service

So you would like to see a breadcrumbs service and a breadcrumbsRepo service?

purely responsible for providing a way for the routes to collect the breadcrumbs models and pass them through to the view ready to by picked up by your component

What are the "breadrumbs models"? The { label, args } POJO? So breadcrumbsRepo would be generating the small POJO from arbitrary models?

Your breadcrumbsRepo service could have methods like findByAllocationId(id), findByTaskGroupId(id) etc etc.

I don't think a single service having nuanced code for turning any model type into every possible breadcrumb is a good idea. This service would balloon over time and become a "god object". With an interface like this, where the only provided arguments are id, it would be even worse. This service would also have to know how to collect models from the store?

Right now you are building up your breadcrumbs manually, and this could all then be done in the same service instead of in all your routes, and potentially moving forwards if you want to create them some other way, this could all be dealt with in the same service or 'repository'.

I don't see how that is a benefit? So a future diff may be 1 file changed, 20 lines changed instead of say 3 files changed, 20 lines changed? I'd much rather have the code organized across files.

---

I think a breadcrumbsRepo service would be an unnecessary abstraction and only be a cause of indirection. Having services like this diminishes the responsibility of Routes in a way that goes against the conventions of the framework.

Routes are meant to bridge the gap between URLs and application state. Since breadcrumbs are a function of a URL (or at least a function of location in an application), they are aligned with this bridge between URLs and application state.

Services meanwhile have no defined purpose. They are the "miscellaneous" datatype. Anything and everything could go in a service, which makes them a poor way to organize an application. They are certainly useful and I'm obviously not advocating an anti-service stance since this PR introduces a service. Just as a general practice--much like being cautious of coupling is a general practice--you should be cautious of over-using services.

You could easily create entire applications with services alone, leaving Routes and Controller with nothing left to do but be in the right place for the framework to call into them. But at this point, why use the framework?

[johncowen]

Hey @DingoEatingFuzz

I'm finishing for the day, but couple of points.

There's nothing I'd particularly 'like to see', I'm more bouncing ideas around and offering a different viewpoint in case it helps - if it doesn't then no problem!

I've just checked deleted the entire nomad repo here locally, re-cloned and checked out the same f-ui-refactor-breadcrumbs branch, re-installed the yarn deps and started up the app again.

I don't get the same outcome as before, as far as I can see, but this time I get this happening:

Anyway, speak Monday 👋

[DingoEatingFuzz]

Hm. I haven't seen that state, but it could be a bug. Did you get there by clicking through a job to the allocation?

[johncowen]

Hey @DingoEatingFuzz ,

I just tried again now. Not sure if this the only way, but I changed the namespace to 'namespace-1', then went into Clients. Clicked the top client, then clicked the top allocation. When I did this the second place in the breadcrumb trail was blank, like the image above.

Thinking on, should the breadcrumb read 'Clients > ClientID > AllocationID' when I go this route?

Thanks,

[DingoEatingFuzz]

This has been rebased against master to include the bugfix in #4475. You should no longer be able to get to an allocation with a missing breadcrumb!

Thinking on, should the breadcrumb read 'Clients > ClientID > AllocationID' when I go this route?

It makes more sense given the architecture of Nomad for allocation breadcrumbs to extend the job/task-group trail. This also isn't new behavior to this PR.

[johncowen]

Apologies for the wait! I've had a good click around an I always get filled up breadcrumbs now so 👍

[github-actions]

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.