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

Update mobx to 6.9 and mobx-react to 9.1 #7211

Draft
wants to merge 7 commits into
base: 2.6
Choose a base branch
from

Conversation

alexander-schranz
Copy link
Member

@alexander-schranz alexander-schranz commented Nov 25, 2023

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Fixed tickets fixes #
Related issues/PRs #
License MIT
Documentation PR sulu/sulu-docs#

What's in this PR?

Update mobx to 6.9 and mobx-react to 9.1

Why?

With the support for decorators again in mobx we could smoothly upgrade to latest version.

To Do

Comment on lines +135 to +136
makeObservable(this); // this should be enough that we still should be able to use legacy decorators

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should make the above variables observable, but the beneath test fails expect(isObservable(router.route)).toBe(true) as router.route is still not observable based on isObservable:

Bildschirmfoto 2023-11-29 um 00 10 47

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this error maybe related to some babel issues as I tried now define the observable via makeObservable directly and run into the following error:

Bildschirmfoto 2023-11-29 um 01 00 40

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strange is that it still output the route is defined:

but as expected it is undefined:

Bildschirmfoto 2023-11-29 um 01 19 15

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are some subtleties in how Babel defines its class fields. From top of my head in 'loose' mode, it only allocates the field on first assignment, so you might want to set this to 'strict' (or the other way around, it has been a while). Explicitly initializing it might also help, e.g. route: ?Route = undefined.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also you probably want to check expect(isObservableProp(router, "route")), otherwise it checks if the value stored in the route field is observable, not whether the property itself is.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx @mweststrate for your response. It really seems to be that in this case isObservable is failing but your suggested expect(isObservableProp(router, "route")) works. So in Mobx 6 (or 5) only the class itself knows if its observable property not the property itself?

It look like / I hope it is only used in tests and so I can maybe just do a replace via regex to convert existing isObservable to isObservableProp:

\(isObservable\((.*)\.(.*)\)\)\.
(isObservableProp($1, '$2')).
Bildschirmfoto 2023-11-30 um 10 24 06

Following changes seems to make the Router.test.js work: b8cd486. Seems I need to check this on a lot of other tests.

If props and params are never itself observable is the replacement of isArrayLike not required:

-if (!isArrayLike(values)) {
+if (!(Array.isArray(values) || isObservableArray(values))) {

and could it just be:

+if (!(Array.isArray(values)) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect your old tests where just testing the wrong thing, and that somehow the value got observable or not. Whether that should be the case probably differs per case. E.g. what you were testing previously was const x = a.b; isObservable(x). Sometimes x was I guess supposed to be observable, sometimes maybe not (e.g. built in objects not managed by MobX etc.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Array.isArray should now work on MobX observable arrays indeed as well

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

Successfully merging this pull request may close these issues.

None yet

2 participants