Sources Tree View

[jlongster]

This is a bigger PR, but hopefully it makes sense. I'm rushing to open this PR because I want to get this out there, but I plan on going through the code to see if there's anything to clean up. One thing I know I want to do is add more comments to document this.

This implements an efficient tree renderer of the sources. I went through several ideas to get here:

• First I had a simple function that took a list of sources and generated a nested data structure, and rendered it with the tree.js component added here (the one from the memory tools). This worked, but I thought we should move the tree-shaped sources data back into the reducers.
• I implemented the tree generation in the sources reducer instead. This gave me better access to specific events, like a single source being added. But it also made me try implementing it with immutable.js, and while it was rather elegant, it just seemed too slow. There's a lot going on, and profiling didn't really show any specific bottleneck, but I don't feel good about keeping the tree immutable when it's something only our component needs. It's rendering data. Through this process I also realized something: we get a lot of newSource events. We can't re-create the whole tree on each newSource event, so it's important that we incrementally update the existing tree structure as fast as possible.

(Note: we call loadSources which gives us a big list of sources, but if the debugger hasn't seen any of those sources before if will fire a newSource event before our loadSources call finished. That means when you open the debugger you get a ton of newSource events and the loadSources result. This is really confusing on the old debugger so I wanted to make that more clear)

• This is what I ended up with. I moved the tree generation back to the Sources component; the reducer doesn't know anything about the tree shape, it just adds sources to the Map. The Sources component has internal state which is a mutable nested tree data structure. When it gets a new sources Map, it diffs it with the old one to figure out which sources has been added, and adds those specific sources to the tree and rerenders.

What's nice about this is that the reducer can add the same source multiple times, but if that happens it's a no-op on everything but the first add. We can also control exactly what happens with the tree structure strictly based on the sources Map; if it's an empty Map we clear the tree structure (happens on a reload). We could support removing sources, or anything we want, by analyzing the source objects. Deriving this information straight from there provides a nice division of responsibilities: the reducer just adds to the Map and the component is responsible for generating the data it needs to render.

This shouldn't break any replay, logging, or other functionality because the SourcesTree component should generate the tree structure on the fly.

Let me know if you have any questions.

[jlongster]

Also, I'd like to merge this in, not squash it. These commits represent the points in history of my 3 ideas and I'd like to keep that in the history. I might want to refer back to various things I implemented in each of them (the way I generated the tree structure also changed in each of them).

[jasonLaster]

What's the benefit here?

[jlongster]

I'll remove it, I was only using it will heavily debugging. Calling renderToolbox within then makes the entire rendering happen within a promise, so any errors will be gobbled up. We show unhandled errors in devtools but only when the promise is garbage collected, so you have to wait a few seconds (or even longer in worst cases).

Anyway, I hate promises because they are like a virus. They spread everywhere.

[jasonLaster]

Thanks for the write up. I appreciated getting your thought process. I agree that maintaining local view state is a good balance. Also, I doubt mutating the tree will be a bottleneck.

I didn't study the tree logic too much, but it seems fine. It'd be nice to add some unit tests here soon as well.

[jasonLaster]

Feel free to merge when you're ready.

[jlongster]

Mutating should not be the bottleneck, yeah, but when I was using persistent data structures it was a little more worrisome. Although several things were going on at that point, particularly this: in debugTab it was yielding on actions.loadSources() which means it blocks the entire UI until the sources are finished loading. I was testing cnn.com which had thousands of sources and it took a long time for the UI to show up. I think most of the perf problems I was experienced was due to this. Eventually I removed the yield so the UI shows immediately and the source listing updates over time as new sources come in.

I used our performance tools and when there are thousands of sources, the thousands of newSource events really clogs the websocket. But I guess there's nothing we can do about that. Showing the UI immediately makes it feel much more responsive, even if it takes a while for the listing to completely flesh out.

[jlongster]

I will write at least some basic tests before merging this.

[jlongster]

I moved the source tree data structure into a utility lib to clean up the Sources component file, I've been meaning to do that. Now it's easy to test also. Added some basic tests for the tree structure. Things are passing so I'll merge this now.

I will work on more tests. I also might tweak the tree structure a little now that I'm more familiar with the problem.

[gregtatum]

I think some code comments here would be really helpful on why this ManagedTree is needed and how some of the handlers are flowing through. It's a little hard to un-pack what is going on here, even though I know you mentioned to me briefly that you needed this. I'm finding the props on here to be particularly confusing. It somewhat breaks the metaphor in my head of how props get passed in from above, because this component is essentially shadowing the the parent's props. The only two properties that are overwritten are the onFocus and renderItem. Could these be different names, more explicit names to make this clearer?

I don't know if things could be refactored a little bit to make it clearer what's doing what... It could just be a matter of being more vigorous with comments for some context.

[jlongster]

It's essentially a higher-order component. It's quite useful to do this sort of stuff in React, but how we use it definitely should be documented.

The Tree component that we use has no component state (well, right now I think it does track the window height but I want to remove that). It doesn't keep track of the current focused item, or which nodes are expanded. It's all up to the consumer to handle that. It's nice to do it that way for several reasons: it makes tests easier to write, and it allows the consumer to store that info somewhere else like redux if they want to.

But it puts a lot of burden on the user, especially if they are trying to learn how to use the tree. This is a common problem with components: who manages the state? A good technique is to offer two variants of the same component: one that doesn't manage the state, and one that does (here, we are calling it "managed").

So we're just wrapping the Tree widget (so we can't change the prop names, we are passing all of that into the Tree) and handling the necessary state for it to work.

In practice, React has more going on than just "props passed down, events go up". For ease of use it's local component state is really nice. Also, there's really no reason it has to be separated from Redux state. At some point React should allow the consumer to "mount" the internal state of any component somewhere else; you can read about it in this React issue.

Anywhere, we're still figuring out the best patterns here and we'll definitely document them more.