[MobX-react] Allow React class components wrapped by Observer HoC to be extended

[skiritsis]

Hello,

We're using MobX and MobX-react in a couple of very large scale projects. One of the projects includes a list of React components for UI elements such as inputs, dropdowns, etc. that are being used by the others.
These components can be either used "as is" so they are wrapped by Observer HoC, or be imported and extended in the rest of the projects to modify their behavior and render functions depending on their needs.

The problem with MobX-react 9.x.x is that this is strictly not allowed, see:
https://github.com/mobxjs/mobx/blob/main/packages/mobx-react/src/observerClass.ts#L62

    if (componentClass[isMobXReactObserverSymbol]) {
        const displayName = getDisplayName(componentClass)
        throw new Error(
            `The provided component class (${displayName}) has already been declared as an observer component.`
        )
    } else {
        componentClass[isMobXReactObserverSymbol] = true
    }

In previous version this was simply a warning, see:
https://github.com/mobxjs/mobx-react/blob/master/src/observerClass.ts#L25

    if (componentClass[mobxObserverProperty]) {
        const displayName = getDisplayName(target)
        console.warn(
            `The provided component class (${displayName}) 
                has already been declared as an observer component.`
        )
    } else {
        componentClass[mobxObserverProperty] = true
    }

Seems like the warning and tracking of components that are already observables was added in response to: mobxjs/mobx-react#839

In even older versions of MobX-react this could be accomplished through "wrappedComponent" property available in wrapped components with Observer to access the original component.

I've raised a similar issue in the past to address a warning for our project use cases here:
mobxjs/mobx-react@b095407#diff-b97ac37a763a2cc028e2da311ed32ce9b4941b59a39b1ae619f9f335d9526749

And this test is a good example of we are using the components:
mobxjs/mobx-react@b095407#diff-ebddfbd06fdc3c5239aa8f53eb90b15da8c6f940b3214edbe685fafff1ceae88

Are there any ways in the latest version to achieve this?

Ideas that come into my mind to resolve this:

1. Handle it in user-space, by exporting 2 instances for each component. One wrapped with observer and one without.

class ObservableBaseField extends BaseField {};
export default observer(ObservableBaseField);
export { BaseField };

2. Monkey patch MobX-react, by overwriting how "makeClassComponentObserver" works and not throw when parsing components being already observers.

3. Have a utility method in MobX-react that can "undo" an observer component and restore to its normal state? This could be done either inside Mobx-react itself, when it detects that a component is already an observer or in user-space. For example, in components that extending a component which is already an observer:
   export default observer(convertToNormalComponent(ObservableBaseField));

Any feedback would be much appreciated!
Thanks!

[mweststrate]

I'm rusty on class components, but I'm not sure you need to wrap observer around subclasses again, so this might be sufficient?

export default observable(BaseClass)

...

export default class MySubClass extends BaseClass

For inheratince reason it might be that you need to protect the render function, so you end up with something like:

@observer
class BaseClass {
  function render() {
    return this.renderImpl()
  }
  
  function renderImpl() {
   return <div />
  }
}


class SubClass extends BaseClass { 
  // don't override render!

  function renderImpl() {
   return <text />
  }
}

[skiritsis]

Thanks @mweststrate for your reply! This is indeed a workaround we might have to resort to, unless there is a better alternative.
The reason we want to avoid is that this would have to be "enforced" over hundreds of components. @marnick1 provided a couple of more examples of the issue below.

[marnick1]

Hello,

In previous mobx-react versions, up until v7, it was possible to inherit from observer parent classes and redeclare the children as observers. Overwritting the render function was working correctly, and it also did not trigger any errors, as shown in this example:
https://codesandbox.io/p/sandbox/mobx-react-7-inheritance-dgz4xw?file=%2Findex.js

In newer mobx-react v9 however, the exact same code throws the following error:
The provided component class (Child) has already been declared as an observer component.
Removing the observer property from the child component and overriding the render function causes it to not re-render at all, as shown in this example:
https://codesandbox.io/p/sandbox/mobx-react-9-inheritance-ff234l?file=%2Findex.js

As you mentioned, if we dont override the render function directly and instead use a renderImpl, it works correctly without the need to re-declare the child as observer.
However, in our case it's impractical to apply this workaround to multiple large-scale projects, as we would have to change the render method of hundreds components, and to remove the observer property from just as many children components for it to work.
Also, this solution looks kind of "hacky" for it to become the norm of the codebase.

Are there any other solutions where we can avoid doing this?

Thanks for your feedback!

[marnick1]

Note: The renderImpl() solution seems to work for v7 as well, so it seems that the only thing that has changed is the fact that the child component can not be re-declared as observer.

[mweststrate]

For the renderImpl, it should be fairly easy to codemod it (see mobx-undecorate package for inspiration). Note that you can also just disable the exception by using patch-package. I just guarantee in that case if things keep working or fall apart. The reason for the exception was probably more robustness against different kind of compilation flags in Babel / TS that compile classes slightly differently in terms of how and when things are initialized.

[urugator]

The behavior in previous versions was not well defined, so now it's explicitely forbidden:

I don't recommend simply disabling the check.

Handle it in user-space, by exporting 2 instances for each component. One wrapped with observer and one without.

Recommended atm. I would investigate the possibility of declaring the base component as abstract.

In even older versions of MobX-react this could be accomplished through "wrappedComponent" property available in wrapped components with Observer to access the original component.

IIRC wrappedComponent was only available on inject/observer(func)?, because class components are not wrapped, but patched.

Actual support is open for disccusion, but it requires an investigation and a test suite.

[skiritsis]

Thank you both! For the time being we will proceed with exporting 2 instances of each component, ideally however we would like a solution within the library itself to avoid this.

We'll check mobx-react source code a bit in the next weeks and try to come up with ideas/solutions or at least some tests in order in order to have clear view of the problem.