You might want this. The comment seems to indicate that useLayoutEffect is necessary in the browser. I don't have full context though

I talked offline with @benschac about this, and shared some feedback about submitting diffs like this. If you're making a substantial change to something like this, you should either (1) make sure you have the full context on the decision, or (2) reframe your diff as a proposal, avoid confident assertions like "useLayoutEffect isn't needed here", and provide much more context in the diff description for what you're solving and why you think this is the right decision.

That said, the other side of this is that @benschac and I don't fully understand why useLayoutEffect is necessary here. I appreciate @def-au1t's explanation above, but unfortunately after reading it I am still pretty confused. We discussed this briefly in the weekly sync, and I came up with three things I think @def-au1t can do to make the explanation more clear:

1. Be clear that this issue only applies when there one menu that is being rendered is replaced by another menu, with the two menus controlled by two distinct components
2. If this issue only applies to the first render, it's not clear to me why it's relevant, because the first render doesn't have the menu open. Would be good to explain this
3. There's no video of the issue, so it's hard for somebody to really understand what the problem is

@benschac introduced a new stack... it might be best to abandon this one and follow-up on D3681?

That said – @def-au1t, I still am curious to understand a bit better why useLayoutEffect is necessary here

In D3678#101545, @ashoat wrote:

@benschac introduced a new stack... it might be best to abandon this one and follow-up on D3681?

That said – @def-au1t, I still am curious to understand a bit better why useLayoutEffect is necessary here

I'm going to! Same here, I'm still trying to understand @def-au1t's comment above. A gif/screenshot would be helpful. Going to give it another read momentarily then abandon this diff.

Also appologies @def-au1t -- I should have asked and framed a question rather than made an assertion, additionally should have left more of a description in this diff.

closing in favor of D3681

The issue I had using useEffect is presented on video below:

The problem source was, that we were calling renderMenu before updatePosition, which was called in useEffect, so the position was correctly set after in the next render, so the menu was shown in a position of previously rendered menu for a short period of time (it was nondeterministic and happened only from time to time, as it's in the video). The solution was to execute effect with updatePosition synchronously, so the position was updated in the same render which contains calling renderMenu.

However, it seems to me, that the issue is no longer valid. I wasn't aware, that this behavior was accidentally removed in the last major iteration of D3377 diff introducing the menu: https://phabricator.ashoat.com/D3377?vs=10741&id=10819

Finally, as a part of this diff, we moved calling renderMenu from useCallback to useEffect, what result is that both of them are executed from useEffects. If React executes both useEffects containing these function calls in the same render - what I suppose happens, updatePosition should no longer be called after renderMenu, and its position should be initially correct.

I did some tests to confirm, if using useEffect is safe now, and I didn't notice any issues anymore.
It was my mistake, as I was absolutely sure, that we still call renderMenu before updatePosition (what was causing the issue for over a half of month, when I was working on it) and I'm really sorry for that.

No worries! Thanks for the detailed description.