Bug fixes and Plugin Versions content

[zrthxn]

Fixes: #199
Fixes: #230

Versions Content

Add list of versions to content section and add choice of which version to install in the install button.

[zrthxn]

@jennydaman After this is merged, the master branch can be deployed.

[jennydaman]

I noticed that the screens for "plugin meta" (/plugin/pl-name) v.s. "plugin" (/p/14) are the same. This is okay for now but we will need to make them distinct in the future.

I think I said this before, the "Install to ChRIS" url should be something from the backend API's data. It should not be built from string concatenation/template. On top of that, the template you are using is not correct for /p/<N> routes.

[jennydaman]

Also note that all backend API handles must end in trailing slashes. However, you shouldn't have to worry about this if you are using the client correctly, that is, avoid concatenating URLs and instead use the URLs as they are returned by the backend.

[zrthxn]

The URL was only being built for the /plugin/pl-name route. Otherwise I am using the URL from the response to show in the box. I'll fix this right now.

[jennydaman]

I finally got around to understanding how this all works, and a few notes.

The <Switch> in Plugins.jsx is nested under the component exported by Router.jsx right? Having a nested router which manipulates routes at the same level (here, both are manipulating top-level routes) is highly prone to errors. I suggest refactoring such that the application only has one top-level <Switch>.

Regarding <Plugin>: optional props are okay, and here pluginData is optional. But self-fulfilling optional parameters is poor design. When arguments are not given to an object (component), the object should either show a default or do nothing (component is blank), it should not try and fulfill its own arguments (props). This is especially true in the world of React because you're blurring the line between props and state, where props are not mutable by the component itself.

On top of the "self-fulfilling parameter" the if-else of componentDidMount causes further branching of logic leading to high code complexity.

In OOP there is a concept "inversion of dependency," where it is the client's responsibility to coerce data to the object's interface. In React speak, the parent component in Plugins.jsx (more specifically, whichever Switch > Route is the parent to <Plugin>) should be responsible for wrangling its data to match the expected props for <Plugin>. In effect, you should move the complicated network requests and data (pluginData) coercion code within the execution tree of Plugin.componentDidMount (so also consider fetchPluginVersions and fetchPluginData) to its parent (which in your implementation, the parent is defined in Plugins.jsx). The goal would be for Plugin.jsx to define a stateless component which does nothing more than display the data its given, without trying to modify nor create data in any way. It would be the sole responsibility of Plugins.jsx to provide the correct data. This responsibility of "providing the correct data" should happen in one place and one place only. If you want to support the /plugins to /plugin/pl-name data saving feature, and you want <Plugin> to be nested under <Plugins>, then <Plugins> should be responsible for providing the correct data to <Plugin> in all cases, and <Plugin> should be a "dumb" stateless component which operates with what it is given and does nothing else.

[zrthxn]

@jennydaman Okay so to understand better

1. Why are nested routes like this a bad idea? I'd have thought that it's necessary to do this to have different components rendered by route. I've used this in the Chris ui app too. So I don't know how it'd cause problems. I'd need to know if I am to fix it.

2. We aren't self-fulfilling props exactly. Yes we're fetching data which wasn't provided in props but we're using the state to do this. In any case, if we were to use plugin.jsx to just render then its parent, Plugins.jsx would need to also handle fetching by ID which would mean that it'd have to either guess what type of param is provided (either string name or number ID).

3. The if-else can be made to just be if. The else part can be common to all renders, meaning the plugin page would show the versions tab even if I opened a plugin by ID (populated by other versions).

I think if one component was to do all the data coersion it would first mean that that component would be too big and certain methods in that component would only be used when children are rendered, not when it's own content is rendered. So that would probably go against separation of concerns.
Moreover, if the same reasoning was to be applied to Plugin.jsx and PluginBody.jsx then the body should only render and the Plugin.jsx component should fetch the readme and all of those things which are currently done in PluginBody.jsx. Would that be right?

[jennydaman]

1. In terms of tradeoffs to make, having nested routers isn't that bad. We should keep your solution if you think it works best.

Here is the reason I would argue it is convoluted (not ideal):

The <Switch> is a handler of sorts, which responds to when the location is updated (/plugins to /plugin/pl-z2labelmap). Let's abstract the parts here:

• URL's location as SourceA,
• <Switch> in Switch.jsx as HandleA
• <Switch> in Plugins.jsx as HandleB
• The HTML as SinkA

When there is one <Switch> which routes top-level locations, we have the following

(SourceA) --[listened to by]--> <HandleA> --[updates]--> (SinkA)

Let's start on common ground and agree that having only one <Switch> is a paradigm with zero complexity.

What about when we have two Switches, which listen on the same resource, and affect the same resource?

(SourceA) --[listened to by]--> <HandleA> --[updates]--> (SinkA)
   \                                                        /
    --[listened to by]--> <HandleB> --[updates]-------------

In this situation, complexity is increased because HandleB and HandleA share the same source and sink. If HandleA updates HandleB, it is now possible for HandleB to enter an undefined state.

This is a simplification of our situation because in reality, you have SinkA and SinkB where SinkB is a child of SinkA. But the point stands.

The ChRIS_store_ui and the ChRIS_store are relatively simple applications and it's preferable to not have to make these kind of software design decisions and tradeoffs if we can avoid them.

[jennydaman]

we're fetching data which wasn't provided in props but we're using the state

2. Exactly what I am criticizing, you are blurring the line between props and state.

If you weren't given a certain piece of data as a prop, you shouldn't be trying to find the data yourself and then storing it as a state.

If a category of data is being represented by a prop, then the component should consider that piece of data immutable.

This is a design choice that you must go all-or-nothing: do you want

A) Plugin to be responsible for fetching its own pluginData, or
B) Plugin to be "dumb," have pluginData be a prop, where Plugin is incapable of fetching itself, and it's the sole responsibility of Plugin's parent to supply it with pluginData?

Hint: if you are interested in the feature where data from /plugins can be reused when navigating to /plugin/pl-z2imageslices, go with the latter design option.

[jennydaman]

So that would probably go against separation of concerns.

You are trying to use the code for showing details about a plugin for both showing plugin details or plugin meta details, where plugin != plugin meta. So we've already bit the bullet (bitten? bullet has been bit?)

[jennydaman]

The most ideal but time-consuming way forward would be to develop two separate screens for plugin vs plugin meta details. If you don't want to/don't have time for that. then we have to do some sort of data cohesion somewhere, and we are debating where is best for this code to be. My argument is that the code for data cohesion should not exist in the Plugin.jsx file because of dependency inversion. It should be the role of its parent, which is coded up in Plugins.jsx. If you think the amount of code in Plugins.jsx is too much, then create a PluginMetaAdapter.jsx which is imported by Plugins.jex

[jennydaman]

3. The if-else can be made to just be if.

It's not a big deal, I am just pointing out this if statement because it indicates that the interface to Plugin.jsx is a union data type.

I would prefer it if the if/else can be moved outside of Plugin.jsx so that its interface can be static, or something that's at is simpler.

[zrthxn]

The nested Switch was removed and each component only renders itself now.
The following are word descriptions of what they do

• Plugins.jsx => Shows a list of all plugin metas
  params { None }
  returns { List of Plugin Metas }

• PluginMeta.jsx => Given name, shows a plugin meta and its other versions
  params { Plugin name from URL }
  returns { Plugin Meta }

• Plugin.jsx => Given ID, shows a plugin and its other versions
  params { Plugin ID from URL }
  returns { Plugin }

Their behavior is now predictable.