forward ref to the wrapped component

[menelike]

Pass ref to the wrapped component.

This might be a breaking change as:

1. It requires at least React 16.3
2. Refs targeting ReactMeteorDataComponent itself now don't work anymore (I thought of something like options.forwardRef for this, but in our large project we do not have even one case where we had to reference ReactMeteorDataComponent itself, so I skipped it)

fixes #202
fixes #189
fixes #162

[StorytellerCZ]

@hwillson Please take a look.

[holmrenser]

@dburles suggests adding this to a beta release in #262. Anything I can contribute to make this happen?

[menelike]

@holmrenser thanks for your offer.

As far as I'm concerned, this PR is ready to be merged. I'd suggest to not wait for #262 to finish as there is still some work to do #262 (comment)

[holmrenser]

Cool! Since #262 is my main interest, I will try to help out there.

[menelike]

@holmrenser #262 needs to catch up with the goal of this PR, maybe worth to be picked up by you?

[yched]

So, #262 changes the withTracker() HOC to return a function component instead of a class component, so that it can leverage the new useTracker() hook introduced there, instead of soon-to-be-deprecated componentWillXxx() lifecycles at the moment.

Since function components can't receive refs, that would actually cause a BC break for existing code passing a ref to the HOC-wrapped component,. There are probably little actual use cases for doing that anyway, since as you point out above such refs currently reference the ReactMeteorDataComponent itself, but I do have one app that does just that (using the ref for ReactDOM.findDOMNode(), which works fine with the ref pointing to the ReactMeteorDataComponent - could/should be done differently now, since findDOMNode() will go away at some point, but point is, such code exists)

If this PR here gets merged and the official behavior becomes "withTracker forwards the ref to the decorated component", then this becomes a non-issue for #262, since React.forwardRef doesn't care about function vs. class components.

@dburles : how do you think we should move forward ?

• For now I kept the BC break in Expose useTracker hook and reimplement withTracker HOC on top of it #262, just adding a comment pointing here.
• I could include the forwardRef behavior in Expose useTracker hook and reimplement withTracker HOC on top of it #262, but it's a behavior change that is probably better committed in a PR of its own.
• I could preserve strict BC in Expose useTracker hook and reimplement withTracker HOC on top of it #262 by temporarily adding a 2nd, class-based wrapper around the function-based one, which would accept existiing refs. It could be removed when this PR here is merged.
• Or if this PR here gets merged first, I would just rebase on top ;-)

[menelike]

@yched thanks a lot for your feedback, you've got a very valid point regarding breaking the backwards compatibility. Did not have that case in mind, thanks a lot!

Since #262 will result in a major release and breaks BC anyway, I'd vote for a solution where #262 supersedes this PR or a new PR targeting #262 like you proposed

I could include the forwardRef behavior in #262, but it's a behavior change that is probably better committed in a PR of its own.

Whatever we do, I think it makes a lot of sense to bundle the breaking changes from this PR and #262 into one release.

wdyt @yched ?

[menelike]

Superseded by eb55a16 from PR #262