Convert RawTable to a function component

[robertbrignull]

Converts the RawTable from a class component to a function component.

Paired on this with @norascheuch and @shati-patel.

As far as I can tell, all the behaviour still works. The only complicated bit is the scroller. You can trigger this behaviour by using the codeQLQueryResults.down command (and similar commands for other directions) to navigate around the raw results table. When the selection goes off the screen it'll scroll to bring it back to the centre.

Checklist

• CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
• Issues have been created for any UI or other user-facing changes made by this pull request.
• [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

[robertbrignull]

I think the behaviour we want for the scroller is we want one scroller per RawTable instance, and we don't want to create a new one every time we render. So I'm using useMemo to cache and instance but then calling scroller.update() directly every time we render. Is this ok? It seems to work. Internally the scroller keeps a shouldScrollIntoView field so it'll only actually do anything when it has previously been instructed to do so.

Since the ScrollIntoViewHelper class is used from another location I don't want to go too hard on refactoring it right now, but once we've also dealt with PathTable we could update the scroller to fit with a functional component better, such as ditching shouldScrollIntoView and being triggered using a useEffect hook on selectedItem.

[koesie10]

I think it would be slightly better to call useEffect here to make it explicit that we're calling this method on every render. useEffect without any dependencies is run on every render as well, but I think it will check if a re-render is necessary after it, so it should be more correct:

Suggested change

	scroller.update();
	useEffect(() => scroller.update());

[robertbrignull]

Ah, I had forgotten that useEffect without any second argument at all (or I guess implicitly passing undefined) had that behaviour. I agree that would be better.

I remember always feeling like the empty array and undefined should be the same here, but actually there's complete opposite behaviours. Just got to remember that.

[robertbrignull]

I'm not sure why we were requiring ResultTableProps as props but then barely accessing any of the members. I've changed this to just require databaseUri. The place where we call RawTable we do {...this.props} and this still works since unknown props are just ignored.

[shati-patel]

Thanks! Had another look and tested locally. Seems fine to me!

Since we paired, and since I don't really understand the scroller situation, I'll let someone else review/approve 🙇🏽‍♀️

[koesie10]

General structure looks good and I was able to trace back all code the original code in the class component. I mostly have comments about the code style and the use of useMemo/useRef/useEffect

[koesie10]

I think it would be slightly better to call useEffect here to make it explicit that we're calling this method on every render. useEffect without any dependencies is run on every render as well, but I think it will check if a re-render is necessary after it, so it should be more correct:

Suggested change

	scroller.update();
	useEffect(() => scroller.update());

[koesie10]

useRef is probably better for this since it's made for keeping things such as this. The behavior is probably identical, but useRef will guarantee that this value is not cleared and will not cause a re-render.

[robertbrignull]

Good point. I must admit I shied away from useRef because I'm less familiar with it.

The react docs say the following about useMemo:

You may rely on useMemo as a performance optimization, not as a semantic guarantee. In the future, React may choose to “forget” some previously memoized values and recalculate them on next render, e.g. to free memory for offscreen components. Write your code so that it still works without useMemo — and then add it to optimize performance.

So that makes it pretty clear we want to be using useRef here.

[koesie10]

(If you take my suggestion of using useRef for the scroller, the RawTableRow should probably also receive a ref. I don't think changing this to scroller.current would be correct)

[robertbrignull]

I don't quite understand the reasoning here. Could you explain a bit more? My thinking is that passing scroller.current here should be the thing to do.

My understanding of useRef is that when the value changes it does not trigger a redraw. So if the value did change we wouldn't re-run this code. We also don't ever change the value of scroller anyway so it doesn't really matter, but let's ignore that.

Let's say we did change the value and it did redraw for some other reason. In that case we want to pass a new value to RawTableRow because the scroller object itself won't have changed so it won't see that it needs to redraw RawTableRow too and not use a cached value. By passing scroller.current we would be passing a new object and it would re-render RawTableRow correctly.

Or is there another reason that I'm missing?

[koesie10]

I'm actually not totally sure about this. The documentation mentions "Do not write or read ref.current during rendering." That would imply that we shouldn't read scroller.current anywhere, not even when we pass it along. I think one way around this is the following:

1. Pass the scroller ref into the RawTableRow
2. Make the RawTableRow accept scroller?: RefObject<ScrollIntoViewHelper>;
3. Instead of using ref={props.scroller?.current?.ref(isSelected)}, we should create a wrapper around this so we don't read props.scroller.current during rendering. We should be able to do this using ref={node => props.scroller?.current?.ref(isSelected)(node)}, but this doesn't work because the functional refs don't interface well with the class-component refs.

I think this would be the ideal situation to avoid the pitfall mentioned on that documentation page, but we can't do that for now. I would suggest just using the current implementation (i.e. setting scroller={scroller.current}) and working on fixing this potential bug later.

[robertbrignull]

That's interesting. I didn't know you could pass a function to that ref= property. I'm not totally sure if that counts as "during rendering" of not.

My understanding of the danger is that it's around changing ref data and because a component is not entirely defined by its props and state. That means we avoid most if not all of the danger because we don't ever change the ref. I think what we have now is probably ok but just ugly. I'm ok with leaving it as it is for now and coming back to fix it later.

Another different approach I thought of could be f91f7aa, avoiding using a ref entirely and instead look up the element by id. I'm not 100% sure this is safe either, since I wonder when the DOM is changed and when useEffect hooks run.

[koesie10]

I believe the DOM is already changed when useEffect is run. If you want to run something before the DOM is updated, there's another hook useLayoutEffect that's made specifically for that purpose. I'm happy to go with this solution for now, but removing the ref and making this easier to understand would definitely be my preference.

[koesie10]

LGTM