Adding observer affects original component

[sergei-startsev]

What is the current behavior?
Adding observer wrapper affects original render method. There's some regression after upgrading mobx-react to version 6 caused by affecting the original (unwrapped) component.

class Foo extends React.Component {
  render() {
    //...
  }
}

const observableFoo = observer(Foo);

observableFoo.render as well as original Foo.render is overridden by adding

<Observer>
    <Component />
</Observer>

Since observer isn't pure, there's no possibility to reuse a component without mobx.

Here's a repository with repro steps: https://github.com/sergei-startsev/mobx-react-observer

What is the expected behavior?

There's an expectation that adding observer wrapper should not affect the original component. E.g. adding observer wrapper in the default export below should not affect named Foo export:

export class Foo extends React.Component {
  // ...
  render() {
  }
}
export default inject('store')(observer(Foo));


// Foo shouldn't be wrapped
import { Foo } from './Foo.js';

[danielkcz]

I don't follow what's the issue. If you use exported Foo, it's unaffected by observer and as such can be reused without knowing anything about MobX. If you use default export in tests, it will have Observer in there, otherwise, it won't be there. I wouldn't consider it impure. Using the same input gives the same output.

Can you elaborate more please?

[urugator]

Seems to be done deliberately, check this PR
d3820da#diff-3035dd5a37335cc50db60f91ad838797
Maybe there is some reasoning in the original PR:
#523
And it's also mentioned in this comment (last sentence):
#588 (comment)

[sergei-startsev]

@FredyC

If you use exported Foo, it's unaffected by observer and as such can be reused without knowing anything about MobX.

It is affected, take a look at Foo > shallow test from repro steps. It uses named unwrapped Foo export, however its snapshoot contains Observer component:

- <div>
-   Test
- </div>
+ <Observer>
+   <Component />
+ </Observer>

[urugator]

Is this a viable workaround?
export default inject('store')(observer(class ObserverFoo extends Foo {}));

[danielkcz]

@sergei-startsev I am looking at a snapshot file and I don't see any Observer there. That's why I am confused.

https://github.com/sergei-startsev/mobx-react-observer/blob/master/__snapshots__/Foo.test.jsx.snap#L9

[sergei-startsev]

@urugator
Yes, it works, but it doesn't look like a workaround though. I suppose the original render method shouldn't be overriden:

mobx-react/src/observer.js

Line 140 in 4904b62

target.render = function renderWrapper() {

It seems to me that a new inherited class has to be created instead of changing prototype of the original:

mobx-react/src/observer.js

Line 126 in 4904b62

const target = componentClass.prototype || componentClass

[sergei-startsev]

@FredyC
Take a look at mobx-react 6 step: https://github.com/sergei-startsev/mobx-react-observer#mobx-react-6

[danielkcz]

I see your step, but writing something in readme does not make it true 😆 Why test snapshot is showing something else?

[urugator]

It seems to me that a new inherited class has to be created instead of changing prototype of the original:

Have you checked my first comment?
There was a PR doing what you're suggesting, but the PR was quickly reverted after a month or two.

[sergei-startsev]

@FredyC I've added both examples with mobx-react 5 and 6 to demo different results. See the issue description:

There's some regression after upgrading mobx-react to version 6 caused by affecting the original (unwrapped) component.

Just run steps from mobx-react 6 to see the diff.

[danielkcz]

It would be much more helpful if the snapshot would be showing an actual problem.

Either way, check the discussion on why the change you are suggesting was reverted. If you have idea how to make that work, PR is most welcome.

#523 (comment)

[mweststrate]

See also #703, this behavior might need to be reverted anyway (not that it really does make a fundamental difference, v5 is also patching classes, but at least the behavior would be the same)

That being said, this problem is unavoidable from an implementation level, as we have to reach into some of the React internals. If you want a pure declaration: don't use observer, but use Observer instead. That is the "proper" composition abstraction React, but not without it's own downsides, such as making it hard to coordinate life cycle events between the "outer" component and reactive rendering in the "inner" component.

[mweststrate]

Fixed in 6.1.0

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs or questions.