fix: Check for forwardedRef in withGlobalEvents

[tofumatt]

Fix #7707

Description

If a component isn't setting a ref then forwardedRef will be null and the block will crash. This checks for the ref first.

How has this been tested?

Ran it locally and things seem fine, and it fixes the problem in the issue.

[gziolo]

This sort of makes sense. @nerrad, how do you handle this in your PR?

[nerrad]

Well the pr is simply passing through forwardedRef to the components. I think HOC's will still need to handle the forwardedRef similarly (depending on how ref is used within the component the HOC is wrapping).

[nerrad]

So to be clear, currently the createHigherOrderComponent is just passing along forwardedRef through the HOC to the WrappedComponent. This means, that it's still possible that forwardedRef is null. What I could maybe do is set forwardedRef to noop when ref is null?

[jorgefilipecosta]

This seems to fix the problem in my tests, and given the severity of the bug, we should probably merge it.

That said I'm not certain if this is the best fix or not as I still don't have total context about what caused the problem. Maybe we can merge this for now and if we have a better solution later we can improve.

[mcsf]

Let's merge for the fix, then have a better plan for refs.

[tofumatt]

Thanks!

If folks would preferSetting it to no-op we could do that, but it seems the same as checking for truthiness and only running the ref callback then… assuming this is the only place we deal with forwarded refs.

The no-op solution is more consistent and I considered it, I guess it's better in principle.

[nerrad]

The no-op solution is more consistent and I considered it, I guess it's better in principle.

If your comments are in response to what I wrote above, I think what you have here is fine for now. I was mostly just thinking that the solution in the forwardRef work being done in #7557 could just set forwardedRef to noop if ref is null (thus wrappedComponents wouldn't need a conditional check)

[tofumatt]

Yeah, I'm just thinking it's nicer to just set it as a no-op function by default instead of conditionally checking it.

I think #7557 should go that route, it's easier to follow in my mind. 👍