Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
phiresky opened this issue Jan 1, 2024 · 6 comments
Open

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

phiresky opened this issue Jan 1, 2024 · 6 comments
Labels

Comments

@phiresky
Copy link
Contributor

phiresky commented Jan 1, 2024

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
Copy link

domehead100 commented Jan 28, 2024

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.

  • To get around this for now, I just exported an object from my root store, "stores", and am importing that into the inject3.ts file. This is not ideal for testing, though Jest can rewrite imports when testing and also the stores will be overridden if passed in as direct props just like the existing inject decorator.

  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
Copy link

NogrTL commented Jul 29, 2024

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
Copy link
Contributor Author

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
Copy link
Collaborator

kubk commented Jul 29, 2024

@alexfoxy
Copy link

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
Copy link
Collaborator

kubk commented Sep 20, 2024

@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.

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

No branches or pull requests

5 participants