Skip to content
This repository has been archived by the owner on Dec 31, 2020. It is now read-only.

Observable props of @observer-ed component #124

Closed
Strate opened this issue Sep 13, 2016 · 18 comments
Closed

Observable props of @observer-ed component #124

Strate opened this issue Sep 13, 2016 · 18 comments

Comments

@Strate
Copy link
Contributor

Strate commented Sep 13, 2016

As for now, this.props object itself is not an observable, which is a problem if we trying to use it inside computed function or bount child component. Some examples:

@obeserver
class Component extends React.Component {
  @computed
  get calculatedValue() {
    return this.props.value1 + this.props.value2
  }
}

claculatedValue does not tracks this.props properties, because it is not reactive. And the developer ran into unobviously issue, "why mu computed function does not recompute?"

Second one is bit more complex, but has almost same behaviour:

@obeserver
class Component extends React.Component {
  childComponent = observer(props =>
    <div onClick={this.props.onChildClick} />
  )

  render() {
    return <this.childComponent />
  }
}

If Component receives new prop onChildClick, this function is not assigned to child div, because child component does not rerender.

As for now, I'm working on custom decorator, which made this.props of component observable. I'll show you my results, and we'll have a little discuss about it :)

@mweststrate
Copy link
Member

Yeah I definitely see why you want this :)

I can see three options

  1. another decorator which makes this.props observable (downside: another decorator)
  2. by default introduce this.observableProps (downside: redudancy, ultimately confusion)
  3. make this.props by default observable (downside: risk, subtle unforsee-able bugs, like having a prop that receives a plain object that must stay plain? use asFlat, might be confusing as well?)

@andykog
Copy link
Member

andykog commented Sep 13, 2016

  1. Make @observer configurable, something like:
    @observer({ inject: [], observableProps: true })

@mweststrate
Copy link
Member

just took a shot at this one, by trying to make the this.props an observable reference. However that doesn't work out, because even if shouldComponentUpdate returns false, the nextProps props are still assigned to the component instance. So a straightforward extendObservable(this, { props: asReference(this.props)) doesn't work out. Could be wrapping a getter / setter around it that only accepts new props if sCU or something, but it might become quite brittle.

An alternative observableProps property, and updating that one in componentWillUpdate or componentWillReceiveProps might be easier, but sounds a bit confusing at the same time

@mweststrate
Copy link
Member

cool, just figured out how to do this properly!

Can be tried with build: [email protected]. Not sure whether this is a really good or really bad thing to do :) but all tests succeed with it. Writing componentShouldUpdate is now even more pointless then ever.

@Strate
Copy link
Contributor Author

Strate commented Oct 17, 2016

@mweststrate thank you for investigation on this!
Unfortunately, there is a bug.
If function passed to the props, calling this.props.propName inside render method of component invokes that function. Seems that this happens because of asFlat usage here. Instead, consider to use asReference on each property of props.

@pelotom
Copy link

pelotom commented Oct 19, 2016

As an alternative to this approach, I've been using the following simple utility in my own code:

function mobxWrap<M>(Wrapped: React.ComponentClass<M> | React.SFC<M>) {

  @observer
  class MobxWrapper extends React.Component<{ model: M }, void> {
    render() {
      return <Wrapped {...this.props.model} />
    }
  }

  return MobxWrapper
}

Which can be used like so:

type Person = { firstName: string, lastName: string }

// Wrap a stateless component -- pure simplicity!
const Stateless = mobxWrap(({ firstName, lastName }) => (
  <h2>Hello, {firstName} {lastName}!</h2>
))

// Wrap a stateful component
@observer
class Greeting extends React.Component<PatientHeader, void> {
  @observable private age: number = 0

  constructor() {
    super()
    setInterval(() => { this.age++ }, 1000)
  }

  render() {
    return (
      <div>
        <h2>Hello, {this.props.firstName} {this.props.lastName}!</h2>
        You are {this.age} seconds old!
      </div>
    )
  }
}
const Stateful = mobxWrap(Greeting)

I was considering making a microlibrary for this function, but thought I'd propose incorporating it into the mobx-react project directly. What do you think?

@pelotom
Copy link

pelotom commented Oct 20, 2016

Until such time as props themselves can be observable, I've made a library for the above utility:

https://github.com/pelotom/mobx-component

@pelotom
Copy link

pelotom commented Oct 21, 2016

Never mind, I just discovered this approach has the flaw that @computed properties don't come through. I also just discovered #136 where it looks like you've figured out a more well-rounded solution to the problem!

@Strate
Copy link
Contributor Author

Strate commented Oct 21, 2016

@pelotom could you please try use [email protected]? I need to know if there is any issues.

@pelotom
Copy link

pelotom commented Oct 21, 2016

Hm, it doesn't seem to work for me. Here's my test:

import * as React from 'react'
import * as ReactDOM from 'react-dom'
import { observable, computed } from 'mobx'
import { observer } from 'mobx-react'

class Props {
  @observable x: number = 0
  @computed get y() { return this.x + 1 }
}

@observer
class Component extends React.Component<Props, void> {
  render() { return <div>x={this.props.x}, y={this.props.y}</div> }
}

const props = new Props()

ReactDOM.render(<Component {...props} />, document.getElementById('root'))
// Should render:     <div>x=0, y=1</div>
// Actually renders:  <div>x=0, y=</div>

props.x++
// Should render:     <div>x=1, y=2</div>
// Doesn't re-render

@Strate
Copy link
Contributor Author

Strate commented Oct 21, 2016

@pelotom thank you for feedback.
As for your 1st case (where actual render is not the same as expected) - looks like a bug. Is it works with 3.8.4 stable version?
As for 2nd case - this is not intended to work - PR didn't fix it. You still need to dereference inside observer, not outside (as you do). I don't think that it is possible at all to get this case to work.

@pelotom
Copy link

pelotom commented Oct 21, 2016

Ah, then I've misunderstood the point of this issue. I thought the point was to make it so we didn't have to wrap observables in a prop, but could pass an object with observable fields as the entire set of props. That's what I'm trying to do with https://github.com/pelotom/mobx-component.

@Strate
Copy link
Contributor Author

Strate commented Oct 21, 2016

@pelotom unfortunatelly, I think it is not possible to do without wrapper (as you suggested), or without some code transformation (such as a babel plugin).

Btw, by default, babel converts <Component {...props} /> into <React.createElement(Component, props). As you see entire props object passed to component's this.props. Seems like React itself perform obtaining shallow copy of props object somewhere.
But <Component {...props} a="1" /> converted to React.createElement(Component, {...props, a:"1"}) - dereference happens before react receiving props.

@pelotom
Copy link

pelotom commented Oct 21, 2016

Well good, I guess I didn't waste all my time making that :)

@mweststrate
Copy link
Member

Can be tested with [email protected]

@mweststrate
Copy link
Member

Shipped with 4.0.0 :)

@mnpenner
Copy link

@mweststrate What specifically shipped with 4.0? Can we make props observable now?

Specifically, I want to pass an observable down from a HOC, which I presently can't seem to do without putting it in a prop and boxing it.

@Strate
Copy link
Contributor Author

Strate commented Mar 13, 2018

@mnpenner mobx-react@4 was released in 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants