WIP: Plugable NavTabs implementation candidate

[daltonmatos]

Hello,

Here is a Pul lRequest with an idea of implementation of issue #4169. The ideia here uses the Event firing approach.

What have been done

• js/constants/tabs.js was replaced with a ne NavTabStore (js/stores/NavTabStore.js);
• All places that imported tabs from js/constants/tabs.js now imports the new NavTabStore and call its getTabs() method.
• A new function was registered in PluginDispatcher, this functions is intended to be used by plugins that want to append a new navtab. This functions generates a new event, NavTabEvents.CHANGE. The ideia of this event is to re-render all components that relates to navtab rendering;
• js/components/TabPanesComponent.jsx was refactored to extract each tab to a new React component (Deployments tab was already extracted, so only Applications tab was extracted and generated a new component: js/components/AppsAndGroupsListComponent.jsx);
• New tests were added to check that appending new tabs to the internal array works as expected. Also check that events are correctly fired;

What still needs to be done

• How the new route should be registered? Show the plugin have access to the Router Component thorugh window.marathonPluginInterface? Maybe a better approach would be to register the new route automatically after appending the new tab to the internal array. This is possible because the Tab ID is also the Tab route. Maybe a future modification could allow a Tab to choose which React route it wants to use.
• Add the listeners to the right places, so whenever a new tab is appended the right components should be re-rendered.
• Write a prof of concept plugin that adds a new tab. Maybe add this implementation to the example plugin?

Final considerations

Let me know what do you think. Don't worry about the commits, I will squash any commit as needed, just say it. Also I will rebase this code when needed. This is a work in progress, is here for revision and suggestions.

Thanks a lot.

[jdef]

/cc @orlandohohmeier

[orlandohohmeier]

Hey @daltonmatos,

thanks a lot for your proposal! Please excuse my late response, we're currently busy cutting the 1.8 DC/OS release and haven't found much time to review PRs.
I had a first look and liked the way you used events to register tabs. We will have an in-depth look later this week.

[daltonmatos]

Hello @orlandohohmeier,

First of all, thank you very much for your time to take a quick look at this proposal. I'm glad you liked it at the first look. Don't worry about the late response, take your time to look at it more deeply so when can discuss all the technical details.

I also still have to think about the routing implementation. I have 2 approaches in mind:

1. The new route is registered as soon as the new tab is added to the internal tab array, that is, inside NavTabStore.appendTab(), to be honest I don't know if the NavTabStore is the best place to mess with the routes. Another point is that the routes would be registered before the NavTabEvents.CHANGE is fired. I don't know if this is a problem, actually.
2. The new routes would be registered by a code that listens do the NavTabEvents.CHANGE event. This seems better but have an important drawback: the code that listens to this event does not know which tabs were just added and it would have to re-register all routes, every time. Maybe creating a new event specific for "route registering" would do it. If we do this we could fire this new event for each new tab added, different from the NavTabEvents.CHANGE event that is fired once for each batch of tabs added.

These are just loose ideas that passed through my head, let me know what do you think about this part of the implementation. I'm sure we will find a good solution to this.

Thanks a lot!

[daltonmatos]

Hello all!

I added one more commit that makes us closer to a fully functional code. Now the core of marathon-ui listens to the NavTabStore.CHANGE event and adds the new tabs correctly.

I took some time to realize that there are two PluginEvents.js (inside /src/js/events/ and /src/js/plugin/shared/) and that the one exposed to plugins is located at /src/js/plugin/shared/. So I moved all APPEND_NAVTAB references there.

Does it make sense to unify these two?

This is the patch applied to the marathon-ui-example-plugin project:

diff --git a/src/main/js/main.js b/src/main/js/main.js
index bb925f6..f496570 100644
--- a/src/main/js/main.js
+++ b/src/main/js/main.js
@@ -1,7 +1,18 @@
 import ExamplePluginComponent from "./components/ExamplePluginComponent";

 const {PluginHelper, PluginMountPoints} = window.marathonPluginInterface;
+const {PluginDispatcher, PluginEvents} = window.marathonPluginInterface;

 PluginHelper.registerMe();
 PluginHelper.injectComponent(ExamplePluginComponent,
   PluginMountPoints.SIDEBAR_BOTTOM);
+PluginDispatcher.dispatch({
+  eventType: PluginEvents.APPEND_NAVTAB,
+  data: [
+    {
+      id: "/mesos",
+      text: "Mesos",
+      component: ""
+    }
+  ]
+});

And after re-building the plugin and copying the resulting .jar to the right place, there's a brand new tab in Marathon UI!

The last step is the route registering implementation. When this is done we will be able to fill the component field and confirm that the rendering works.

Let me know what you think.

[daltonmatos]

Hello @orlandohohmeier,

I tried some approaches on the dynamic route implementation but I think I will need your help on this, not just with ideas but maybe with some code.

I tried to use the router that the TabPanesComponent.jsx receives on contextTypes. On the same method that listens to NavTabStore.CHANGE I tried this:

+    var newroute  = new Route({
+      name: "Mesos",
+      path: "/mesos",
+      handler: DummyComponent
+    });
+    this.context.router.addRoutes(newroute);
+    this.context.router.refresh();

I discovered the addRoutes() method looking at the this.context.router on Chrome console. But this did not work, unfortunately.

Another approach was to create a new wildcard Route (inside js/main.jsx), like this.

+import PluginRouteComponent from "./components/PluginRouteComponent";

 var routes = (
   <Route name="home" path="/" handler={Marathon}>
@@ -17,6 +18,7 @@ var routes = (
     <Route name="taskView" path="apps/:appId/:view/:tab"
       handler={AppPageComponent} />
     <Route name="deployments" path="deployments" handler={TabPanesComponent} />
+    <Route name="plugin" path="plugin/:id" handler={PluginRouteComponent} />
     <Redirect from="/" to="apps" />
     <NotFoundRoute name="404" handler={PageNotFoundComponent} />
   </Route>

This means that any tab added by a plugin must use the path /plugin/<tabname> and not just /<tabname>. In this implementation the PluginRouteComponent would look at the current path and find wich tab is responsible for that path, and then would render its provided component.

This works, but is not the way I wanted it to be. This limits the power of what a plugin can do and would probably invalidade a possible "Login Plugin", for example, since such plugin would very likely want to hijack the root route (/).

So I would like to have your help on this part of the implementation, more specifically:

• Help to confirm that react-router allows the insertion of new routes after it is fully loaded and running;
• If not, maybe we could refactor the way marathon-ui currently builds the Router, and maybe we could create a new event that would re-build the Router, passing a new array of routes (this array would include new routes from new tabs). Don't know the real implications of this, just an idea;

Thanks a lot!

[daltonmatos]

Hello @orlandohohmeier,

Do you think that splitting this PR in two would make this implementation more likely to be merged? The idea would be:

1. One PR with the refactoring of the tabs array, already with the new NavTabStore but without any of the event logic;
2. On PR with the events implementation.

Let me know and I will split the code and open the PRs.

Thanks,

[orlandohohmeier]

Hey @daltonmatos, I'm really sorry that I still haven't found the time to look into the details of your PR - I had to prioritize bug fixes over features. 👼
We set aside one day every week to look into Marathon UI issues and PRs, and I hope that we find time next week to finally review your PR and support you so that we can land this feature.

Let's not split this into different PRs as the number of changes is rather small and I don't think that it will help streamlining the process.

I can't say this enough, your work is highly appreciated and more than welcome!

[daltonmatos]

Thanks a lot @orlandohohmeier !! I fully understand you!
In the mean time I will work a little more on the dynamic route implementation, I had some more ideas about this. I will experiment with some of these ideas and update commits here as needed.

Again, thanks a lot for taking your time to look at this.

[daltonmatos]

Hello all,

I'm putting this PR on hold because of the announcement about the discontinuation of marathon-ui in favor of DC/OS-ui. Commit: 05ce810

[orlandohohmeier]

@daltonmatos I'm really sorry that we didn't manage to land this feature. Thanks again for your work!

[daltonmatos]

No problem @orlandohohmeier! It was, in fact, very fun to write this code, learn about ReactJS and the Marathon-ui code base.

I plan to implement this same feature in DC/OS-ui. I just have to reserve some time do study the code and start the implementation.

[orlandohohmeier]

@daltonmatos Hope you're ok with me closing this PR as we first need to updated react and react router to enable the needed capabilities to properly implement this feature.

[daltonmatos]

No Problem, @orlandohohmeier. I will come back to this implementation in the future. Maybe there's a way to do it without havbing to update react and react-router. But we can discuss this in another issue. Is it better to open the issue here or on JIRA?