Yomguithereal / baobab-react

React integration for Baobab.
MIT License
309 stars 38 forks source link

How to UT baobab branch components. #68

Closed briandipalma closed 9 years ago

briandipalma commented 9 years ago

I'm just wondering how you UT a component that is wrapped in a baobab HOC? I could export the stand alone component from the module but then I have to pass in the tree as part of the context.

I noticed that react router now passes in children and params as props, I wonder if taking that approach in baobab HOCs would make testing easier?

Yomguithereal commented 9 years ago

Intuitively I'd say you probably want to UT the unwrapped component itself since it only relies on its props & state if you don't bring the tree in. But this makes the component export a bit difficult since you have to branch it somewhere else through HOC.

Would it be complex for you to provide a context to your component when UTing it?

Yomguithereal commented 9 years ago

@briandipalma: could you elaborate on react-router's strategy please? What I want to avoid is the user having to explicitly pass the tree as a prop from the top of your component hierarchy to the bottom.

briandipalma commented 9 years ago

https://www.youtube.com/watch?v=Q6Kczrgw6ic at about 14 minutes you can see them talking about passing in params as a prop and the children that each router managed component is meant to be rendering. I'm not sure exactly how it works but I imagine they pass in the data as props from their HOC. So what you'd do is pass tree and cursors on this.props instead of this.context.

I think it's cleaner for UTs and makes sense, at the end of the day your component is going to be bound to Baobab in some way. You may as well use props for everything including accessing the tree and cursors.

briandipalma commented 9 years ago

I can provide a context with withContext but that is deprecated so I'm not sure how I can UT a component that relies on context in a forward compatible way.

Yomguithereal commented 9 years ago

Ok, I see how react-router does. The problem with the mere props approach is that, even if this is convenient in a router case where you won't have to propagate the params across more than one or two component, in Baobab's case this becomes nuts because you'll have to propagate the tree from the top to the bottom of your component's hierarchy and as a typical app has at least 8-10 levels, this is really a drag.

However, if you do use, HOC, here is what I could do: let you access the original component of the composed component through HOC. This one indeed only relies on props without the tree.

Example

/* component.jsx */
class MyComponent extends Component {
  render() {
    return <span>Hello {this.props.name}</span>;
  }
}

const ComposedComponent = branch(MyComponent, {
  cursors: {
    name: ['name']
  }
});

export {ComposedComponent};

/* test.jsx */
import {ComposedComponent}

// Accessing the original component for easy UT:
const MyComponent = ComposedComponent.original;

However, I think that generally, the pattern followed by Flux users in this case is to separate clearly clever from dumb components so you only test the latter ones.

briandipalma commented 9 years ago

So in the higher order branch component what you can do is this

render() {
      return <Component tree={this.context.tree} cursors={this.context.cursors} {...this.props} {...this.state} />;
    }

This makes the component rely on props and not context so making it easier to UT if exported without the HOC wrapper. The HOCs can deal with the drag of passing in the tree and cursors instead of having the component author do it. Does that make sense?

Yomguithereal commented 9 years ago

Yet it makes sense. But do you need the tree or the cursors in the Component?

briandipalma commented 9 years ago

Different components need different things, if you pass in cursors={this.context.cursors} and there are no cursors used in the wrapped component, no harm is done as this.props.cursors wont be accessed in the wrapped component. There is no need to know if the wrapped component uses the tree or cursors or not, just pass it in for all cases.

Umm maybe I'm making a mistake in my architecture. I'm using the cursors and tree for their event emitting properties so they can replace a dispatcher. Maybe that's the wrong approach? Thing is I'd then have to pass in a dispatcher down all my components. It would be neater if Baobab's HOCs simply passed in the tree and cursors as props and let me decide to use them when/if I need them.

Yomguithereal commented 9 years ago

Yes, using the Baobab event emitter to add an action layer and update the state is a fine idea and I used to do so myself. But lately, I switched to raw functions that are easier to live reload. My component then require said functions. And they will pass them to dumb components as props typically.

briandipalma commented 9 years ago

The issue with using functions for action/event processing is that you can't bind to a components state. Say for example you have 10 instances of the same component. You can't use a stateless function to update a specific component on a user action, unless you close over the components cursor?

Even then it's less flexible then emitting events as anything can register for those events so if a user of your component needs to update some state following an event all they need to do is register for the event. While if you use a function they can't. I guess that's not a concern if you control all of an applications code...

I might explore this approach and see if it helps simplify my code. Thanks for the suggestion.

Yomguithereal commented 9 years ago

My approach would be to create functions taking the tree and optionally some other arguments then pass actions to the tree and curry them by giving them the tree through context in clever components or something of the kind. But I must find time to experiment this properly.

Yomguithereal commented 9 years ago

Any update about this @briandipalma?

briandipalma commented 9 years ago

I took a quick look at the new docs for Baobab 2.0 and it looks much more testable, all data including actions are passed in as props. Looks great to me, thanks!