WIP: refactor(#125): Added attribute data to context for proactive fetching

[joshiraez]

This refactor proactively fetches data into the context. Done this as part of #10 because I'll be using that to store the Chart polling job and data (as it seemed better and to avoid this logic into the component)

This has some nice advantages:

• Removes the need to use useEffect on the component themselves, being able to be "dumb" (they just operate on the given props/context)
• Allows for proactive logic in table selection
• Separates fetching and mapping logic from rendering. Easier unit testing

Having the Chart data in the context also allows for the Chart data to be maintained while on different tabs (as base HawtIO loses the data when changing to Attribute view). Could also be improved on the future to maintain the data on multiple visited nodes, or whatever we want.

We could move the actions page data to the context as well if that's ok, although that has a bit more logic so I left it out for now.

Putting this up to get some feedback on the design. I left out the types I'll be using for Chart in case you want to give some feedback on them. WDYT?

[joshiraez]

And this is why you needed to use usePluginNodeSelected() 😄

[tadayosi]

Hmm, if I'm not mistaken this doesn't answer why we can't have merged PluginNodeSelectionContext with PageContext.

[joshiraez]

I should add also some tests. I'll be working on those

[tadayosi]

I don't understand why we need this. I don't think this is a right direction. There are two reasons:

• If we remove scheduler registration in the useEffect, how can we get the realtime data of the attributes?
• I see already the circular dependency between the core and plugins as a problem that should be removed. This change even extends to the opposite direction by adding more details of the plugins to HawtioPage. We should at least keep the PluginNodeSelectionContext as only holding the selected node.

[joshiraez]

@tadayosi It's as we were talking about in the other PR, as this allows us to fetch the data proactively and let the components only focus on the rendering part, having the data already gathered.

Also allows for only one point of data gathering instead of having every page have its own job

If we remove scheduler registration in the useEffect, how can we get the realtime data of the attributes?

The idea is to have that Data service take care of the scheduler jobs, instead of every component, which would be the next step. That way every component can always ensure that they have the latest data only accessing the context.

Also helps avoids bugs because if we have () => unregister x in every component in the return function of the useEffect, which is the cleanup, it could happen that it cleans up something that it shouldn't and creates logic dependency between components.

For example with the chart component I'll be setting a Post job request like in base hawtio and store all the data there, but I'll remove all the logic from the component, and allowing us to do logic and store data between components, as base hawtio loses all the data on component switch, making really complicated for the user to have any kind of trackable data unless they don't switch components (in fact, while trying to record a gif with peek, I discovered it loses the data even in a window resize!)

We should at least keep the PluginNodeSelectionContext as only holding the selected node.

I can create a new context that only pertains to the current plugin. For example, in JMXContent. Would that be better for you?

I don't understand why we need this. I don't think this is a right direction

The alternative to this is to keep everything in their own component like now and that would mean that we would have to sooner or later refactor to this design to avoid the discussed bugs.

For now I'll refactor the design to create a context just for JMX as that should solve your worry with the circular dependency

[tadayosi]

@joshiraez No, I won't accept the changes. I don't think you fully understand what you are going to change. Can you instead just create a Chart component that is self-content, reading the mbean attributes within itself? I'd like to ask you to first understand what the old version of Hawtio does in the Chart view and replicate the logic.

[joshiraez]

@tadayosi I can do that but it'll be both slower, have more complexity, and it will have bugs like it happened with the tabular attribute table. Is that okay with you? Just wanting to make sure on the set of expectations you have.

I mean, is just copying this same logic to the component itself. There is no work lost here, as it's the same mapping logic, just that everything will be moved to auseEffect on the component instead of on a context.
Plus I'm decoupling it a bit as the chart component on base HawtIO depended on Core for rendering which I though was one of the goals to achieve.

I'm just mindful about current needs to accelerate to our MVP quickly and address some of the shortcomings that we discussed in the other PR.

Also, what makes you think I didn't read the old version and understand the logic? I'm curious, as I asked that I could pick #8 because I already inspected the whole flow and I'm really confident about the changes, the only thing missing for chart is that you have to filter numeric attributes, set the job, and then aggregate them (and put everything in the chart). It's the second time you brought that and I'm puzzled as I'm trying to provide as much data on the base version as possible without being overly verbose.