mobxjs / mobx

Simple, scalable state management.
http://mobx.js.org
MIT License
27.59k stars 1.78k forks source link

mobx-react @inject does not work with new decorators #3810

Open phiresky opened 11 months ago

phiresky commented 11 months ago

Intended outcome:

Using @inject("foo") with non-legacy decorators should work same as with legacy decorators.

Actual outcome:

throws error class decorators must return a function or undefined (with babel).

How to reproduce the issue:

Nodejs repl:

class Foo {}
const x = require('mobx-react').inject("store")(Foo)
typeof x; // "object"
new x(); // fails

According to here:

“Compatible” means that the returned value must have the same type as the decorated value – e.g., class decorators must return callable values.

Even if React can handle replacing a class with a plain object, new-style decorators seem to not allow it. inject should return a callable / new class instead.

Versions

9.1.0

domehead100 commented 10 months ago

I also came across this when starting a new project and trying to use Inject.

I hacked up a start for a new inject decorator (called inject3 below to indicate stage 3 decorators).

It's not super great at this point, but it does work in basic testing. You have to configure Vite to use the stage 3 decorators, which requires using either Babel or Typescript. I'm using Babel, so this is basically what I did: https://dev.to/kiranmantha/ecmascript-decorators-with-vite-ilg.

There are two specific challenges that I'm aware of right now:

  1. Need to see if there's a way to get the store(s) off of the React.Context (MobxProviderContext).
    • I'm sure it's doable, but can't use useContext() because this is a class component, and can't just use static contextType: MobxProviderContext in the wrapper class because I'm adding the store props to regular props in the Injector's constructor before calling super(propsWithStores), and I can't access the React.Context until super() has been called, but I need the context before calling super() to get the stores from it.
  1. Because of adding the store(s) to the props before calling super(propsWithStore), Mobx itself is logging a warning about passing the same props to the superclass that were passed into the subclass constructor.

I haven't really tested other than it works ala @inject("store") in the small app that I've just started working on, having used it in a couple of components.

I thought maybe we could discuss and maybe someone has some improvements.

I realize that Provider/Inject are supposed to be deprecated in Mobx v7, but I will probably still continue to use them because I am not at all a fan of stateful function components (other when state is via external stores) and am definitely not a fan of hooks. (Signals from Solid, Preact, etc. are better, but still not as good as good ol' Mobx).

Funny how all of these "solutions" have been created over the past several years to problems that I never had as a user of Mobx. Hooks give me almost no advantages because of Mobx, and a lot of disadvantages. I still do web apps the same way that I did back in 2016 when starting with React, getting a lot done without wasting time constantly learning the next "solution." :)

// file: inject3.ts
import { stores } from "../src/_store/store";

type IValueMap = Record<string, any>;

// this is a "decorator factory" that takes in the stores to inject and returns a stage 3 class decorator
export function inject3(
    // first arg could be a function that extracts a specific shape of state from the available stores
    // https://github.com/mobxjs/mobx-react#provider-and-inject
    ...storeNames: Array<any>
) {
    // get store(s) that should be added to props.  These must be provided by the Provider component higher up
    // in the component tree, otherwise an error will be thrown
    // TODO: would it be possible to typecheck store names at build time rather than runtime?
    return function (
        target: any,
        { kind, name }: ClassDecoratorContext,
    ): typeof target {
        if (kind === "class") {
            // return a class that extends the target class with the store(s) injected as additional props
            const Injector = class extends target {
                constructor(props: IValueMap) {
                    // determine which store(s) to inject; explicit props override this behavior
                    const propsWithStores = storeNames.reduce(
                        (acc, storeName) => {
                            if (storeName in props) return acc; // props override injection (useful for testing)
                            acc[storeName] = stores[storeName];
                            return acc;
                        },
                        {} as IValueMap,
                    );
                    // this causes a warning from Mobx about passing a different value into super from what was passed
                    // into this constructor
                    super(propsWithStores);
                    this.displayName = name;
                }
            } as typeof target;
            // rename the Injector to include the injectee name, etc., for ergonmics
            Object.defineProperty(Injector, "name", {
                writable: false,
                enumerable: false,
                configurable: true,
                value: `${name}_With_${storeNames.join("_")}`,
            });
            return Injector;
        }
        return target;
    };
}
NogrTL commented 4 months ago

Same error. Tried to migrate from MobX 3 to MobX 6 and blocked by this.

.babelrc

{
  "presets": ["@babel/preset-env", "@babel/preset-react"],
  "plugins": [
    "@babel/plugin-transform-runtime",
    ["babel-plugin-styled-components", { "fileName": false }],
    ["@babel/plugin-proposal-decorators", { "version": "2023-11" }],
    ["@babel/plugin-proposal-class-properties", { "loose": false }]
  ]
}

Any suggestions how to overcome this without rewriting all @injects?

phiresky commented 4 months ago

We tried some hacks but in the end we migrated to normal react context with const StoreContext = createContext<StoreType>(); <Context.Provider ...>. And then in remaining class based components

    static contextType = StoreContext;
    declare context: StoreType;
    get store(): StoreType {
        const context = untracked(() => this.context); // https://github.com/mobxjs/mobx/blob/main/packages/mobx-react/README.md#note-on-using-props-and-state-in-derivations
        if (!context) throw Error("could not access store. did you provide it?");
        return context;
    }

The "get store" thing is specific to our code but I added it here because that untracked is needed because idk

kubk commented 4 months ago

inject is considered obsolete and it's recommended to use React Context instead: https://github.com/mobxjs/mobx/tree/main/packages/mobx-react#:~:text=Provider%20/%20inject%20to%20pass%20stores%20around%20(but%20consider%20to%20use%20React.createContext%20instead)

alexfoxy commented 2 months ago

inject is considered obsolete and it's recommended to use React Context instead: https://github.com/mobxjs/mobx/tree/main/packages/mobx-react#:~:text=Provider%20/%20inject%20to%20pass%20stores%20around%20(but%20consider%20to%20use%20React.createContext%20instead)

Hooks aren't compatible with class components though. We have a huge project with chained decorators and are struggling to find a solution. We have a mix of class components and functional but would like to avoid re-writing all the class components.

kubk commented 2 months ago

@alexfoxy Hooks aren't required for context, you can use it with class components too:

class ThemedButton extends React.Component {
  static contextType = ThemeContext;
  render() {
    return <Button theme={this.context} />;
  }
}

https://legacy.reactjs.org/docs/context.html

Also check out the phiresky's answer just above my reply in this issue.