Skip to content

Add a setState method. #67

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

Closed
wants to merge 2 commits into from
Closed

Conversation

gordoncl
Copy link

I am interested in adding a setState method that will dispatch the subscribers. My main motivation is to be able to test changes to the state on connected react components in redux without having to create fake reducers and actions.

@dmitry-zaets
Copy link
Collaborator

@Munksey Can you please show one of the tests where you want to use this method?
Just interesting how do you perform testing.
Looks like you are making kind of intergration testing (Redux state -> Connect component -> React component) which is not common and interesting to know.

@gordoncl
Copy link
Author

@dmitry-zaets Sure! I will admit, this is a little experimental.

I have kind of altered the working example to show you what I am trying to accomplish:

/**
 * SearchContainer.js
 */
function createSearchContainer(WrappedComponent) {
  class SearchContainer extends Component {
    constructor() {
      this.state = { pendingRequest: null, query: '' };
    }

    getResults(query) {
      // Filters the entries props.
    }

    handleSearch(query) {
      // Perform actions like maybe cancelling previous request.
      // Deciding if it is necessary to create a new request, etc.
      // Whatever behavior we want.
      ...

      this.setState({ query });
    }

    render() {
      const { pendingRequest, query } = this.state;
      const results = this.getResults(query);

      return (
        <WrappedComponent 
          isLoading={!!pendingRequest} 
          onSearch={this.handleSearch.bind(this)}
          results={reusults} 
        />
      );
    }
  }

  const mapStateToProps = (state) => ({
    entries: getEntriesSelector(state()),
  });

  const mapDispatchToProps = (dispatch) => ({
    searchEntries: (query) => dispatch(someAction(query)),
  });

  return connect(mapStateToProps, mapDispatchToProps)(SearchContainer);
}

export default createSearchContainer;

/**
 * SearchContainer.spec.js
 */
describe('<SearchContainer>', function () {
  function MockComponent(props) {
    return (<div />);
  }

  it('updates the results when the store changes', function () {
    const SearchContainer = createSearchContainer(MockComponent);
    const mockStore = configureStore()({ entries: [] });
    const wrapper = mount(<SearchContainer store={store} />);

    wrapper.find(MockComponent).props().onSearch('pizza');
    expect(wrapper.find(MockComponent).props().results).to.eql([]);

    mockStore.setState({ entries: ['pizza', 'pizzeria', 'spaghetti'] });
    expect(wrapper.find(MockComponent).props().results).to.eql(['pizza', 'pizzeria']);
  });
});

Essentially, my team and I want our connected components to know more about how our actions behave and be more knowledgable about how the app works, as a whole. While, on the other hand, our presentational components know nothing about what actions return or do. So, we are kind of wrapping an intermediate component that can maintain some state and do things like decide if we are in a loading state or not, cancel previous requests or even make new requests.

I know other arguments can be made about putting more state on the store, as well. But, we don't necessarily want this data to persist and it sometimes feels like overkill to create different kinds of context for what triggered a loading event. This is mostly an implementation detail on our part.

I suppose I could decouple these two pieces and test each of them separately, but I like the idea of testing them in unison, as I expect them to always operate together as a unit.

@dmitry-zaets
Copy link
Collaborator

dmitry-zaets commented Aug 25, 2016

Thanks for example!

I don't see any complex connection between store and connected component in your example, expect props.
So you can assume that as soon as your mapStateToProps and mapDispatchToProps works as they should - component will receive all data and function in props as expected.

By assuming this you can split your tests into next parts:

  • Testing reducer selectors (it is easy because they are pure functions)
  • Testing actions and action creators (using redux-mock-store or redux-actions-assertions)
  • Testing mapStateToProps and mapDispatchToProps (also easy as they are pure functions too)
  • Testing that connected component use exact mapStateToProps, mapDispatchToProps and component
  • Test simple component

Decomposing full cycle tests into small unit tests makes them much easier to support and reuse.

@gordoncl
Copy link
Author

Hey @dmitry-zaets! Thank you for looking at the code and the response. I am always happy to get feedback on what I am doing.

I agree that having small unit tests are easier to support. And, we are testing our actions and reducers, separately. The components that are used by this are also tested separately, in the manner you have described.

Here are some of my thoughts and feelings:

I have never thought to test mapStateToProps and mapDispatchToProps. It's not that I don't see it as being invalid or bad, but it just seems like those are internal to your module and not necessarily something that you want to reuse or be apart of your public API. I am not necessarily concerned about how they act alone, but more about the behavior of the connected component. I don't see them as dependencies, but more as private functionality.

In this particular case, I would say that I am treating my connected component as a presenter. A layer between the model (the store) and the view (the wrapped component). These are the two big dependencies that I need to mock to test that my module API is working as expected.

In a lot of other frameworks, such as a rails, you would test your controller by mocking a request and testing to see what data gets passed from your fixtures to your views. This is the kind of behavioral testing I am trying to accomplish.

So, I guess this leaves me with a couple of questions. Am I doing something that is so unorthodox that it is considered bad practice? Are my analogies to other frameworks incorrect, and I am not designing my application in a way that makes sense?

@dmitry-zaets
Copy link
Collaborator

There are no right and wrong ways of testing. If it works for you - it is right :)

Here is a discussion about this topic in redux repo: reduxjs/redux#1462

@gordoncl
Copy link
Author

Since nobody has commented on this in a while and it does not seem to be useful to anyone (aside from myself), I am going to close this PR. This behavior can also be achieved by setting up reducer stubs, so I guess it's not necessary, but more of a helper method.

@gordoncl gordoncl closed this Sep 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants