Breaking changes in Mobx 6

[ajorkowski]

Unfortunately there has been a breaking change with mobx 6 - makeObservable(this) will have to be added to the constructor of any classes that are using the @observable, @action or @computed decorators.

More information and example here:
https://mobx.js.org/enabling-decorators.html

Alternatively the decorators can be removed entirely and makeObservable can be used with a second parameter, which is described in the upgrade guide:
https://mobx.js.org/migrating-from-4-or-5.html#upgrading-classes-to-use-makeobservable

I'm not sure which method you would prefer?

Cheers,
Felix

[foxhound87]

Hello @ajorkowski
thank you for the analysis.
I think we should keep decorators and use makeObservable(this) in each constructor,
this will keep backward compatibility with older mobx versions.

[ajorkowski]

makeObservable does not seem like something that is exported in previous versions of mobx... Would you be happy if we use something like (makeObservable && makeObservable(this)) in each constructor? I think with older versions of mobx the makeObservable function will be undefined if we try to import it. But I think this may also introduce import warnings, so it might be a problem for builds?

This might need a breaking version of mobx-react-form?

[foxhound87]

Yes I don't see any other way to do it.
The linter should not complain it. I have done it for configure/useStrict in the index.js.
We can release it as a feature.

[foxhound87]

I'm trying to update the code but I encountered an issue:

Error: [MobX] Cannot make property '$submitted' observable, it is not configurable and writable in the target object

mobxjs/mobx#2534

[ajorkowski]

So I think this might be related to how you are supposed to call makeObservable in base/parent classes.

According to the docs (and the changes I made in my own code), the base class has to call makeObservable as per normal, and the parent classes also have to call it, after calling super.

So in our case you might have constructors that look like:

export default class Base {
    constructor() {
        makeObservable && makeObservable(this);
    }
}

export default class Field extends Base {
    constructor(...) {
        super();
        makeObservable && makeObservable(this);

        ....
    }
}

Or is this what you are doing and still getting the error? I guess it might be a bug in mobx then :/

[foxhound87]

Yes, that's exactly what I did, I have added it in the Base class as well.
here is the code on a dedicated branch:
https://github.com/foxhound87/mobx-react-form/tree/mobx6-support

[ajorkowski]

Hrm, I was doing more research on this, and they have a page specifically for decorator support. It looks like they expect babel 7 to be used, and there is a difference in the configuration used:

https://mobx.js.org/enabling-decorators.html#babel-7

I'm thinking this is causing the issue - you are using babel 6 which I believe has the equivalent configuration as the expected config with babel 7 for the old mobx, but this new version of mobx uses the alternative class-properties:

{
    "plugins": [
        ["@babel/plugin-proposal-decorators", { "legacy": true }],
        ["@babel/plugin-proposal-class-properties", { "loose": false }]
        // In contrast to MobX 4/5, "loose" must be false!    ^
    ]
}

I guess this makes the upgrade path a bit tricky - ie upgrade to babel 7 (@babel packages), and have a different config for mobx 6 vs others, but this would mean a different build based on the version of mobx at the end of the day.... :(

[vkrol]

@foxhound87 @ajorkowski What do you think about upgrading Babel to 7, MobX to 6 and releasing a new major version of mobx-react-form without support for older versions of MobX? I did it in my fork and I can create the PRs (all tests are green except the performance test).

1. Upgrade Babel to 7: master...vkrol:upgrade-babel-to-7
2. Upgrade MobX to 6: https://github.com/vkrol/mobx-react-form/compare/upgrade-babel-to-7...vkrol:upgrade-mobx-to-6

[ajorkowski]

@vkrol wow awesome, thanks for doing those changes. They look good to me. The only question I would have is why add the configure({ useProxies: "never" }) in the upgrade to mobx 6 branch? This changes how mobx works so it might mess with the mobx config that is being used upstream?

[vkrol]

@ajorkowski Yes, of course, thank you! I forgot to delete it. I will update it a bit later.

UPD: done.

[vkrol]

I created "draft" versions of PRs for visibility:

1. Upgrade Babel to 7
2. Upgrade MobX to 6

[Dakkers]

I think making a new major version here is the right way to go. MobX 3 definitely doesn't need to be supported anymore, and the functionality of 4 and 5 is encapsulated by 6. (including IE support, the only thing worth using Mobx 4 for.) I'm unable to upgrade MobX in my own project until this is done.

[foxhound87]

The mobx6 support has been released in version 3