mobxjs / mobx-react

React bindings for MobX
https://mobx.js.org/react-integration.html
MIT License
4.85k stars 350 forks source link

add more PropTypes #882

Closed MikeDevice closed 4 years ago

MikeDevice commented 4 years ago

Let's imagine the store TodosStore:

class TodosStore {
  @observable todos = [];

  @computed get total() {
    return todos.length;
  }
}

and the component Todos:

import React from 'react';
import ReactPropTypes from 'prop-types';
import {PropTypes} from 'mobx-react';

class Todos extends React.Component {
  static propTypes = {
    todosStore: PropTypes.shape({
      todos: PropTypes.observableArray.isRequired, // or ReactPropTypes.array.isRequired
      total: ReactPropTypes.number.isRequired
    }).isRequired
  }

  render() {
    // ...
  }
}

What's wrong with this example? todosStore.total is a computed number, so the line

total: ReactPropTypes.number.isRequired

triggers a full recompute, as described here https://github.com/mobxjs/mobx-react/issues/56. So I think it would be useful to have the additional PropTypes methods for simple types like number, string, etc.

mweststrate commented 4 years ago

I think it'd be better to have the proptypes in a separate package. It seems they are barely used (and I think TS does a better job at type checking in general)

On Tue, Jul 7, 2020 at 9:20 PM Mikhail Vyrodov notifications@github.com wrote:

Let's imagine the store TodosStore:

class TodosStore { @observable todos = [];

@computed get total() { return todos.length; }}

and the component Todos:

import React from 'react';import ReactPropTypes from 'prop-types';import {PropTypes} from 'mobx-react'; class Todos extends React.Component { static propTypes = { todosStore: PropTypes.shape({ todos: PropTypes.observableArray.isRequired, // or ReactPropTypes.array.isRequired total: ReactPropTypes.number.isRequired }).isRequired }

render() { // ... }}

What's wrong with this example? todosStore.total is a computed number, so the line

total: ReactPropTypes.number.isRequired

triggers a full recompute, like is described here #56 https://github.com/mobxjs/mobx-react/issues/56. So I think it would be useful to have the additional PropTypes methods for simple types like number, string, etc.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx-react/issues/882, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAN4NBBSDZOVXDNFHEIYZCLR2N7RZANCNFSM4OTQ4WZA .

MikeDevice commented 4 years ago

But what to do without TypeScript?

danielkcz commented 4 years ago

@MikeDevice You are most welcome to extract current prop types into a separate package and expand it to your needs. It is indeed used rarely.

MikeDevice commented 4 years ago

Thank you for your answers. I close the issue now.