New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Check for forwardedRef in withGlobalEvents #7710

Merged
merged 1 commit into from Jul 4, 2018

Conversation

Projects
None yet
5 participants
@tofumatt
Member

tofumatt commented Jul 4, 2018

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.

@tofumatt tofumatt requested a review from WordPress/gutenberg-core Jul 4, 2018

// Any component using `withGlobalEvents` that is not setting a `ref`
// will cause `this.props.forwardedRef` to be `null`, so we need this
// check.
if ( this.props.forwardedRef ) {

This comment has been minimized.

@gziolo

gziolo Jul 4, 2018

Member

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

This comment has been minimized.

@nerrad

nerrad Jul 4, 2018

Contributor

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).

This comment has been minimized.

@nerrad

nerrad Jul 4, 2018

Contributor

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?

@gziolo gziolo added the [Type] Bug label Jul 4, 2018

@gziolo gziolo added this to the 3.2 milestone Jul 4, 2018

@gziolo gziolo added the Priority High label Jul 4, 2018

@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

This comment has been minimized.

Contributor

mcsf commented Jul 4, 2018

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

@mcsf mcsf merged commit 6bf9832 into master Jul 4, 2018

2 checks passed

codecov/project 46.46% (+<.01%) compared to 07fe660
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mcsf mcsf deleted the fix/7707-image-embeds branch Jul 4, 2018

@tofumatt

This comment has been minimized.

Member

tofumatt commented Jul 4, 2018

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

This comment has been minimized.

Contributor

nerrad commented Jul 4, 2018

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

This comment has been minimized.

Member

tofumatt commented Jul 4, 2018

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. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment