Move state variable from state to ref to prevent infinite loop

[idastambuk]

This is a fix for an bug with stack overflow in Athena and Redshift that happened when updating grafana to React 18, described here.

I wish I could say with any kind of authority why this happened in the first place, and only in variable editor. @sarahzinger already described here that the problem is in this useEffect:
if (!isEqual(props.dependencies, dependencies)) { setFetched(false); setResource(null); props.onChange(null); setDependencies(props.dependencies);

After debugging, it looks like the

1. props.query gets updated from props.onChange(null) BEFORE setDependencies(props.dependencies) is evaluated.
2. This triggers another run of the useEffect without the dependencies state variable having been updated to their latest value. This means the value is always undefined at the point when this effect runs.

That doesn't explain why it only happened in variable editor and not in panels though, but React's batching 🤷🏻‍♀️ amirite? 😞

I wanted to clean up the state and this effect a bit before I tackled the batching problem, so I followed what is usually recommended in the case of infinite useEffect loops, which is to reevaluate which variables are stored as state variables, and which are just refs. I moved basically all of them from this useEffect (fetched, resource, and dependencies) since they didn't need to be state variables and changes to them shouldn't cause a rerender (AFAI can see).

Aaand this ended up actually fixing the problem! It's the dependencies variable being changed to a ref that did it I think, I assume because it's no longer a state variable, it wasn't being updated (well, NOT updated) asynchronously?
Also, interestingly, if you replace this non-working useEffect with useLayoutEffect it works fine. Maybe it has to do with the synchronicity of useLayoutEffect, but my brain is fried and I welcome someone taking over the thinking for this, if you wish.

This might not be solving an overreaching problem, since I don't know the details of what caused it and in which cases. Im ok with hoping it doesn't happen again by being careful with what we put into state variables when there are a lot of moving parts (like here) from React@18 up.

I also made a small change in CONTRIBUTING.md

[kevinwcyu]

i'm still trying to digest the changes in this pr, but my initial thought is to see if we can try out swapping the Select with AsyncSelect from @grafana/ui. right now it's quite hard to follow the effects, state, and refs in this component.

[sarahzinger]

➕ ➕ yeah I think the only thing this is doing that we don't get with async select is the auto clearing when a parent component changes, but I feel like we've had a lot of bugs with this type of functionality in the past and it might be more trouble than it's worth. I'm cool with us exploring using async select! If it turns out that's a good direction maybe we can deprecate this component and see if we're the only clients of it and if we're ok to remove it from this repo

[idastambuk]

Ok, tried AsyncSelect and already initially encountered what I think makes the switch not worth it:

When an select is cleared, the only way we could trigger a new request for resources in is opening the menu (with old options) and starting to type. This is one of two ways this component triggers a loadOptions, the other is on component mount. So that's now very useful, as there appears to be no good way to trigger a fetch from outside the component. (Or, a non-hacky way that wouldn't possible mess up something else)

I think is the way to go, and it could probably be simplified, but I would much rather merge this now and rethink how we display these dropdowns later (maybe with an invalid border if the selected option isn't valid anymore? dunno)

[kevinwcyu]

yea, thanks for looking. i agree about merging now and rethinking this later on.

[kevinwcyu]

yea, thanks for looking. i agree about merging now and rethinking this later on.