Mobx 6.1 breaks spying on actions in unit test

[heath-freenome]

Intended outcome:

In our unit tests we are using the jest.spyOn() function to ensure that certain actions are called under certain conditions (with expected values) in our components.

Actual outcome:

With the 6.1 release changes all of our spys are failing in the same manner as below

setUsernameSpy = jest.spyOn(authStore, 'setUsername');

results in:

TypeError: Cannot assign to read only property 'setUsername' of object '[object Object]'

How to reproduce the issue:
I'm pretty sure it's obvious how to reproduce the issue. My question is HOW can I verify that an action on an observable is called with the proper arguments now that 6.1 is available? Is there an option I can set to turn off making actions read-only for tests?

Versions
6.0.4 - working
6.1 - not working

[urugator]

Is there an option I can set to turn off making actions read-only for tests?

Not yet, but we would like to adress this somehow and we are open to suggestions.
We would like to avoid a global config flag as that could cause some actually broken code to pass the tests.

[urugator]

We would like to know if it's only about writability of actions or whether we should also expect demand for configurability of actions or other fields (computed/observable).
So if anyone reading this has an experience in testing mobx based codebases, we want to hear from you!
We would like to keep things constrained to keep user in the safe zone, but we also don't want to throw obstacles in the way for legitime use cases.

[vkrol]

@urugator

We would like to know if it's only about writability of actions or whether we should also expect demand for configurability of actions or other fields (computed/observable).

It might be helpful:

Jest should be able to mock non-writable but configurable properties.
Currently jest will throw an error when trying to replace non-writable (but configurable) properties.

jestjs/jest#8656

[heath-freenome]

Are actions still configurable? If so, then maybe before applying the spy I can make it temporarily writable?

[urugator]

Depends if it's plain object or not. All annotated fields of non-plain objects (classes) are non-configurable as well.
We will probably end up with configure({ safeDescriptors: false }) or something like that. Actually already working on it, but still considering alternatives...
EDIT: Note that in case of plain objects, changing property configuration via defineProperty is also potentially dangerous, but not forbidden because we have to support delete key/remove(key) and also it's not something that can happen just by mistake (as opposed to classes).

[heath-freenome]

I'm using classes, so what can I expect?

[urugator]

Non-configurable ... the main reason is that we want to prevent following:

class Child extends Parent {   
   // This is like calling defineProperty with new descriptor
   // It breaks all assumptions we made about the field in parent
   // and there is no reasonable way for us to detect this.
   // So we prevent it by making the field non-configurable in parent
   inheritedProp = 5;
   inheritedArrowFunc = () => {};   
}

[josuemontano]

@urugator I think being able to mock/spy actions is good enough. I cannot think about a use case where mocking an observable or computed is necessary (or even good practice?).

I've being writing MobX specs in large code bases for over a year and never had the need to mock an observable or computed.

Unfortunately version 6.1 broke many specs so upgrading is not an option for us till this is fixed. Thanks for your work!

We would like to know if it's only about writability of actions or whether we should also expect demand for configurability of actions or other fields (computed/observable).

[urugator]

broke many specs so upgrading is not an option for us till this is fixed

@josuemontano Is there anything specific you would like to see addressed or behaving differently?

[heath-freenome]

I'm using classes, so what can I expect?

What I meant was, what can I expect from your solution to this issue?

[mweststrate]

Check the 6.1.2 release notes on 😊

…

On Fri, 29 Jan 2021, 17:21 Heath C, ***@***.***> wrote: I'm using classes, so what can I expect? What I meant was, what can I expect from your solution to this issue? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2752 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN4NBDVVMQCLJAKYP4YGT3S4LVCZANCNFSM4WWF3GEA> .

[ljani]

This seems to break auto-bind as well. Some observations:

• autobind(this); makeObservable(this); will work with @action.bound and seems to work (I guess mobx needs to do the same and thus it is writable)
• autobind(this); makeObservable(this); works once with @action, but breaks when the same class is instantiated the second time (bad)
• makeObservable(this); autobind(this); with @action fails instantly (good)
• makeObservable(this); autobind(this); with @action.bound fails instantly (okay, I guess)

As you can guess, I'm worried about the second case. I wonder if there are any solutions to make it at least fail instantly, other than copying React and running everything twice.

EDIT: I'm mostly interested having all private (non-action) methods autobound, thus makeAutoObservable(this); is cumbersome.

[urugator]

autoBind(this); makeObservable(this); works once with @action, but breaks when the same class is instantiated the second time (bad)

Took me a while to understand why this happens and I've learned something new about JS :D
The first time the constructor runs, the prototype method is turned into an action and becomes writable: false.
The second time when you try to this.action = whatever, it will use inherited descriptor, which says that the field is non-writable therefore the error ... we don't modify prototype with action.bound therefore no error...
I will take a look if we can keep things on proto writable - that does NOT mean that modifying prototype over time is supported! It's just unlikely to happen...

[urugator]

@ljani #2766 should fix this

[urugator]

Can we close this or are there any remaining issues?

For future readers, since 6.1.2 there is a new configuration option configure({ safeDescriptors: false })

[mweststrate]

Closing issue as to many different symptoms are already being discussed. New issues can be reported in a fresh issue with a repro against 6.1.4