meteor / react-packages

Meteor packages for a great React developer experience
http://guide.meteor.com/react.html
Other
572 stars 157 forks source link

Conflicting withTracker docs at guide.meteor.com vs this repo #289

Closed OliverColeman closed 3 years ago

OliverColeman commented 4 years ago

The usage docs/examples at https://guide.meteor.com/react.html#using-withTracker conflict with those at https://github.com/meteor/react-packages/tree/master/packages/react-meteor-data#withtrackerreactivefn-higher-order-component

In the guide.meteor.com docs withTracker does not create a HOC that passes through the props given to it to the wrapped component. Ie to match the docs at guide.meteor.com the line in withTracker.tsx: <Component ref={ref} {...props} {...data} /> would have to be changed to: <Component ref={ref} {...data} /> (Where props are the props passed to withTracker).

I'm not sure which is correct. I initially assumed the guide.meteor.com docs are right, because (I think) that's how the old createContainer behaved, but there's no reason withTracker shouldn't also pass through the props it's given, except...

I do find it frustrating that it requires that wrapped components written in typescript must include all the props passed to withTracker even though they don't necessarily need them. Seems like it would be more flexible to not require this and instead allow the prop type definition for the wrapped component to explicitly include those props if necessary. However, that would be a major API change for anyone who has depended on withTracker passing through the props it's given.

CaptainN commented 4 years ago

I'm not sure I understand your assertion. This:

<Component ref={ref} {...props} {...data} />

Is explicitly passing the props down to the child component - which seems to be what you want. Removing that would not pass props to the child, and would break a lot of code. This is a standard pattern for HOCs (higher order component).

Here's how it works:withTracker is a utility function that produces a HOC factory (a HOC is a factory). It receives a function (a computation handler), and returns another function (the HOC) that can be used to create a container from another component (function or class - not an instance). That was confusing for me when I first saw the pattern, but you can use it to do awesome stuff, like create one HOC, and use it to build multiple containers.

The withTracker function itself does not receive any props. The computation handler passed to withTracker does receive props, in case you want to use prop values in your computation. The computation returns data, which is appended to the child component as props (overwriting same named props - data props take precedence).

Maybe it could be clearer that the docs, but this is all a common HOC pattern implementation.

(Edit: I updated this to correct the use of the term "HOC" - I always get the term HOC mixed up with container, a HOC is the factory, and that produces a container component from another component. The complexity in all this is one of the reasons I prefer hooks - they are much simpler - just functions).

CaptainN commented 4 years ago

I can see that if the guide is suggesting to define props for the child component, maybe it should also suggest to define props for the HOC.

import { Meteor } from 'meteor/meteor';
import { Lists } from '../../api/lists/lists.js';
import { withTracker } from 'meteor/react-meteor-data';
import ListPage from '../pages/ListPage.js';

const ListPageContainer = withTracker(({ id }) => {
  const todosHandle = Meteor.subscribe('todos.inList', id);
  const loading = !todosHandle.ready();
  const list = Lists.findOne(id);
  const listExists = !loading && !!list;
  return {
    loading,
    list,
    listExists,
    todos: listExists ? list.todos().fetch() : [],
  };
})(ListPage);

+ ListPageContainer.propTypes = {
+   id: PropTypes.number.isRequired
+ };

export default ListPageContainer;
<ListPageContainer id={7}/>

This would be more consistent in the guide, but it's not strictly necessary to add propTypes. There are other issues in the guide - it uses outdated React.PropTypes for example...

filipenevola commented 3 years ago

Hi, the guide was updated to focus on hooks.