Skip to content
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

Testable React Components #2559

Closed
iammerrick opened this issue Nov 18, 2014 · 14 comments
Closed

Testable React Components #2559

iammerrick opened this issue Nov 18, 2014 · 14 comments

Comments

@iammerrick
Copy link
Contributor

I wrote an article on dependency injection with react using properties. The TLDR of it is this:

var MyComponent = React.createClass({
  propTypes: {
    model: React.PropTypes.shape({
      getState: React.PropTypes.func
    })
  },

  getDefaultProps() {
    return {
      model: new MyComponentViewModel(new HTTP())
    };
  },

  getInitialState() {
    return this.props.model.getState()
  },

  render() {
    return <h1>Hello {this.state.name}</h1>;
  }
});

This enables a user to override the dependencies at runtime during testing, but leave the implementation untouched. The problem is that if a user has multiple MyComponents their ViewModel is shared. This is a problem using the traditional require() approach as well, where one require('MyComponentViewModel')s because it is a singleton. getDefaultProps() cacheing effectively nullifys this technique's use for instance level dependency injection.

Currently in react there is only two ways to pass things in at runtime:

  1. Child components.
  2. Props

Because React copies props even though they are technically per instance I have no other way of passing in dependencies at runtime.

The ultimate goal of this is to write testable react components without mocking at the module level.

Why not mock at the module level?

Here is an excerpt from my article:

  1. It's slower, you are reevaluating the module several times.
  2. The module still has the same state for an entire test file, multiple it() blocks would still need to reset state or require() the module in each it() block. Slow and difficult to test.
  3. require is a service locator, not a dependency injector. Using it for both conflates it's use violating the single responsibility principle.
  4. In tests uses of instanceof can break because you could be getting a different instance for each require().
  5. Code is now encouraged to be written in the form of singletons which is problematic for it's own reasons.

Some Potential Solutions

I realize that other people may not be of the same opinion about mocking at the module level but react being a library I do think it should have some notion of passing in runtime dependencies. Or somehow support a testing story that doesn't involve module level mocking.

Maybe a constructor lifecycle function that receives the props? This would enable me to per instance use a passed in prop as a dependency. Another option would be to stop caching getDefaultProps(), I imagine that is cached because props are typically viewed as immutable but there is currently no other way to pass arguments to a component.

@iammerrick
Copy link
Contributor Author

cc:// @vjeux

@iammerrick
Copy link
Contributor Author

I tried for several days to use the Angular 2.0 injector but that caters extensively to "everything is a class", and React render functions naturally instantiate their own components make that approach futile.

@Hossman333
Copy link

+1

@iammerrick
Copy link
Contributor Author

And another technique wrapping react by @johnlindquist http://jsbin.com/lujona/7/edit?js,console

@iammerrick
Copy link
Contributor Author

I guess this really boils down for me as two problems:

  1. Why are the default properties copied?
  2. Is it possible to add another lifecycle method for construction time to resolve this issue.

@cascadian
Copy link

I've been using the Angular 2.0 injector with React. Would something like this meet your requirements?https://github.com/cascadian/react-with-di/blob/master/src/index.jsx

@jquense
Copy link
Contributor

jquense commented Nov 20, 2014

question: Why would use getDefaultProps to set this and not getInitialState? It seems like like a misuse of getDefaultProps who goal is not to set instance level props, but provide sane defaults in case of props being missing (no if (this.props && this.props.myprop)). To me, "side loading" a model via default props, breaks the explicit parent/child hierarchy, and introduces a non-explicit entry vector. It seems like it would be better served conceptually in State, which is for instance level stuff. In example, most flux implementations do this by having something like a getStoreState() method that is called onChange and in getInitialState. Is there a reason why you wouldn't want this in state?

@iammerrick
Copy link
Contributor Author

Great question @jquense, the trouble is that the only way to pass something to an instance form outside of it right now is by using props... It is useful to pass things in so you can pass in different implementations at test time. This also forces you to code to an interface instead of an implementation. Mocking at the module level is problematic for the reasons I listed above which is why I am hoping react will facilitate a different style. Does that answer your question?

@iammerrick
Copy link
Contributor Author

@cascadian that is a very cool approach indeed, that is similar to the road I traveled. I ran into the issue though injecting classes, when one a class that was injected might have a dependency of its own, I couldn't get it to work because I needed the class to pass to react create class not the instance. How does constructing a new ViewModel() receive HTTP when you construct it using .

@iammerrick
Copy link
Contributor Author

@cascadian does it feel odd to you using a dependency injector to inject classes instead of instances?

@jquense
Copy link
Contributor

jquense commented Nov 20, 2014

gotcha! @iammerrick I do see the advantage of the simple, "pass in what I want via props, or just use the default" instead of of mocking out a module... Personally I would do something in the middle and just mock getModelState() on my component instead of the mocking the whole model module. Another pattern i've used (albeit more complicated) is to use state when props are not provided.

var model = this.props.model == null ? this.state.model : this.props.model

It might be a bit overkill in this situation but it works great for making controlled/uncontrolled component props.

To the larger issue, React seems pretty set (for better or worse) on their testing story, mostly due to it being the way fb does this, Jest is a good indicator that they feel that module level mocking is the right way to handle mocking in a CJS environment. I definitely see how one cold disagree with that though (I'm not a huge fan either). For me, I spent time trying to figure out a good way to do DI in React, but mostly just kept coming back to "use explicit props and don't rely on defaults" as the best, in practice. Its verbose and repetitive, but that is sort of the React paradigm, "explicitness over abstraction".

@cascadian
Copy link

@iammerrick

does it feel odd to you using a dependency injector to inject classes instead of instances?

No, but I think Angular DI's TransientScope annotation would accomplish almost the same thing if you want to avoid injecting classes.

var HTTP = function(){
  return {
     get: function(){}
  };
}

di.annotate(HTTP, new di.TransientScope());

var MyComponentViewModel = function(http){
  return {
    http: http
  };
}

di.annotate(MyComponentViewModel, new di.Inject(HTTP));
di.annotate(MyComponentViewModel, new di.TransientScope());

I ran into the issue though injecting classes, when one a class that was injected might have a dependency of its own, I couldn't get it to work because I needed the class to pass to react create class not the instance.

Can you give an example? I don't quite follow. In my sample code, Angular DI injects the HTTP class into the MyComponentViewModel "factory" function, so it's in scope for the MyComponentViewModel constructor.

di.annotate(MyComponentViewModel, new di.Inject(HTTP));

Then similarly, the MyComponentViewModel class is injected into the function that create's the React component.

@sebmarkbage
Copy link
Collaborator

I don't buy the arguments listed in "Why not mock at the module level?"

  1. Sure it's slower, I'm skeptical it's too slow. jest is pretty fast and we're using it for many thousands and thousands of test. Do you have a test suite that significantly hinders you workflow because it uses module mocking?
  2. We often use clearCache to reset the cache. This is a limitation in jest. It's not inherent to the model. It could be done automatically for you. Let's fix it. It's not too slow.
  3. Arguably the distinction is arbitrary and it's an academic terminology argument. Not a practical argument.
  4. This is a confusing part of jest because you can require outside of its and inside. We can forbid requires or automatically ensure that each it runs in an isolated scope. It's not a limitation of the model, just the implementation. I'd love to see a pull request to fix this. :)
  5. Arguably over-reliance on statics is a problem but I have yet to see dependency injection automatically solve it. It still requires restructuring. I think that the point you're making is orthogonal to testing because I assume that you want the dependency injection for production runtime to solve those issues.

That said I would like to be accommodating. So I'll try to address your concerns.

First, we're moving toward encouraging shallow testing rather than deep testing. You should be asserting about renders one level deep rather than implementation details of lower level components. See: #2393

That avoids the need to mock nested components. You assert on the ReactElements rather than asserting that the classes get instantiated.

Second, we're moving towards using stand-alone plain classes instead of React.createClass. So you can use any DI system that you would use on top of ES6 classes. I.e. you will be able to just wrap your classes in a DI provider.

Third, your concern about getDefaultProps being memoized is solvable by just wrapping it in a function.

  getDefaultProps() {
    return {
      model: () => new MyComponentViewModel(new HTTP())
    };
  },

Then you invoke the function in getInitialState time. That makes it effectively lazy. The function can capture any additional dependencies in it's closure.

@iammerrick
Copy link
Contributor Author

  1. You may be right that in a CommonJS environment its fast enough. Doing it with AMD was brutally slow. :-)
  2. Not sure what clearCache is, is that for resetting the currently loaded modules?
  3. Fair enough.
  4. Agreed.
  5. Coming from angular land I might be a bit attuned to a DI oriented workflow and just need some getting used to coding without an injector.

Thank you for trying to address my concerns. I appreciate the time you put into your response. You are right about the memoization solution. I am very much looking forward to the move to ES6 classes. :-) Thank you!

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

No branches or pull requests

5 participants