Strict mode error shows up only in development build

[danrot]

Intended outcome:

With MobX 4.15.4 it was possible for us to build our application in both, production and development, modes without any issues.

Actual outcome:

Since MobX 4.15.5 the following error occurs only when building the application in webpack in the development mode, when a certain piece of code is being run:

Uncaught Error: [mobx] Since strict-mode is enabled, changing observed observable values outside actions is not allowed. Please wrap the code in an `action` if this change is intended. Tried to modify: ToolbarStore@41.config.icons[..]._store.validated
    at invariant (mobx.module.js?daf9:29)
    at fail (mobx.module.js?daf9:24)
    at checkIfStateModificationsAreAllowed (mobx.module.js?daf9:767)
    at ObservableValue.prepareNewValue (mobx.module.js?daf9:1085)
    at ObservableObjectAdministration.write (mobx.module.js?daf9:4148)
    at Object.set [as validated] (mobx.module.js?daf9:4312)
    at validateChildKeys (react.development.js?72d0:1662)
    at Object.createElementWithValidation [as createElement] (react.development.js?72d0:1806)
    at eval (Icons.js?be42:30)
    at mapSingleChildIntoContext (react.development.js?72d0:1149)

The line from our Icons.js file mentioned in the stack tract is actually a pretty boring one:

https://github.com/sulu/sulu/blob/4b457cfc291f61759a2a5624ba68aa3ba0cc2d82/src/Sulu/Bundle/AdminBundle/Resources/js/components/Toolbar/Icons.js#L30

I am currently a bit lost, and can't really say what's happening here, especially since it does not happen when built using the production mode in webpack. Because of that I thought that webpack might be the issue, so I created an issue there. But as it turns out downgrading to MobX helps, so I guess it has to be a MobX issue.

How to reproduce the issue:

Since I don't really have a clue why this issue suddenly came up, it is pretty hard for me to deliver a reproduction.

Versions

Happening since MobX 4.15.5.

[danielkcz]

Are you sure you or someone on your team hasn't changed enforceActions to always? We certainly haven't changed a default value.

[danrot]

Yeah, totally sure... It is that line, and if I comment that line out, it works:

https://github.com/sulu/sulu/blob/4b457cfc291f61759a2a5624ba68aa3ba0cc2d82/src/Sulu/Bundle/AdminBundle/Resources/js/index.js#L109

However, if I comment that out, I still get the following react error:

Warning: Cannot update during an existing state transition (such as within `render`). Render methods should be a pure function of props and state.
    in Icons (created by Toolbar)
    in div (created by Controls)
    in Controls (created by Toolbar)
    in nav (created by Toolbar)
    in Toolbar (created by Toolbar)
    in Toolbar (created by Application)
    in header (created by Application)
    in main (created by Application)
    in div (created by Application)
    in div (created by Application)
    in Application
overrideMethod @ react_devtools_backend.js:2273
printWarning @ react-dom.development.js?61bb:88
error @ react-dom.development.js?61bb:60
warnAboutRenderPhaseUpdatesInDEV @ react-dom.development.js?61bb:23250
scheduleUpdateOnFiber @ react-dom.development.js?61bb:21165
enqueueForceUpdate @ react-dom.development.js?61bb:12678
Component.forceUpdate @ react.development.js?72d0:490
eval @ index.module.js?a243:899
Reaction.runReaction @ mobx.module.js?daf9:1805
runReactionsHelper @ mobx.module.js?daf9:1936
reactionScheduler @ mobx.module.js?daf9:1914
eval @ mobx.module.js?daf9:1943
batchedUpdates$1 @ react-dom.development.js?61bb:21856
reactionScheduler @ mobx.module.js?daf9:1943
runReactions @ mobx.module.js?daf9:1919
endBatch @ mobx.module.js?daf9:1618
Atom.reportChanged @ mobx.module.js?daf9:234
ObservableValue.setNewValue @ mobx.module.js?daf9:1103
ObservableObjectAdministration.write @ mobx.module.js?daf9:4164
set @ mobx.module.js?daf9:4312
validateChildKeys @ react.development.js?72d0:1662
createElementWithValidation @ react.development.js?72d0:1806
eval @ Icons.js?be42:30
mapSingleChildIntoContext @ react.development.js?72d0:1149
traverseAllChildrenImpl @ react.development.js?72d0:1007
traverseAllChildrenImpl @ react.development.js?72d0:1023
traverseAllChildren @ react.development.js?72d0:1092
mapIntoWithKeyPrefixInternal @ react.development.js?72d0:1174
mapChildren @ react.development.js?72d0:1198
render @ Icons.js?be42:30
finishClassComponent @ react-dom.development.js?61bb:17160
updateClassComponent @ react-dom.development.js?61bb:17110
beginWork @ react-dom.development.js?61bb:18620
beginWork$1 @ react-dom.development.js?61bb:23179
performUnitOfWork @ react-dom.development.js?61bb:22154
workLoopSync @ react-dom.development.js?61bb:22130
performSyncWorkOnRoot @ react-dom.development.js?61bb:21756
eval @ react-dom.development.js?61bb:11089
unstable_runWithPriority @ scheduler.development.js?3069:653
runWithPriority$1 @ react-dom.development.js?61bb:11039
flushSyncCallbackQueueImpl @ react-dom.development.js?61bb:11084
flushSyncCallbackQueue @ react-dom.development.js?61bb:11072
batchedUpdates$1 @ react-dom.development.js?61bb:21862
reactionScheduler @ mobx.module.js?daf9:1943
runReactions @ mobx.module.js?daf9:1919
endBatch @ mobx.module.js?daf9:1618
_endAction @ mobx.module.js?daf9:1004
executeAction @ mobx.module.js?daf9:958
<unnamed action> @ mobx.module.js?daf9:938
Show 19 more frames

And if somebody would have changed that line, it should also fail when downgrading to MobX 4.15.4, right?

[danrot]

I am also wondering about the ToolbarStore@41.config.icons[..]._store.validated part of the error message... Is _store.validated something MobX internal? If I am not mistaken we do not set any _store property.

[danielkcz]

That React warning is probably related to https://github.com/mobxjs/mobx-react-lite/issues/274

It would really help if you try to set up minimal reproduction, hard to say like this.

[danrot]

As said, I really have no clue why this is happening, therefore it is hard to reproduce it...

Are you sure that warning is connected to the issue you've linked? We are not using mobx-react-lite, but mobx-react@5.4.4.

And maybe my previous comment is a clue? Is that _store variable somethign MobX internal? What can cause it to be updated?

I am also wondering about the ToolbarStore@41.config.icons[..]._store.validated part of the error message... Is _store.validated something MobX internal? If I am not mistaken we do not set any _store property.

[danrot]

Okay, dug a bit deeper, and found a little bit more.

It seems to be related to these lines: https://github.com/sulu/sulu/blob/4b457cfc291f61759a2a5624ba68aa3ba0cc2d82/src/Sulu/Bundle/AdminBundle/Resources/js/views/Form/Form.js#L546-L550

What we are doing here is creating an array of JSX elements, that will be passed to a store in order to be rendered somewhere later. The _store.validate part I was wondering about above seems to come from React. My guess is that with the latest MobX release the _store property also is observable now for some reason, and when React internally tries to change it (this is not done by us) MobX seems to complain.

Does that ring any bells for you? If not I might be able to reproduce that now in a CodeSandbox, although that might take me some time :see_no_evil:

[danrot]

A current workaround seems to be to put a toJS around the place where the JSX I've put in here is rendered. So the code written here:

https://github.com/sulu/sulu/blob/4b457cfc291f61759a2a5624ba68aa3ba0cc2d82/src/Sulu/Bundle/AdminBundle/Resources/js/containers/Toolbar/Toolbar.js#L156

Would be replaced with something like this:

{iconsConfig.map((icon) => toJS(icon))}

Is that a good idea? Should there be an easier way to handle this? Would you consider this a bug or expected behavior? Maybe what we've done here a while back was not the best idea :see_no_evil:

[urugator]

array of JSX elements, that will be passed to a store

Make sure the array or wherever these elements are stored is shallow, otherwise mobx will convert them into observables (because they are plain object).

[danrot]

Since this array is part of a bigger config object stored on an observable property of a store that is not easily possible, so the toJS approach described above is the only quick win I could find. But I am still wondering why this is only happening when built using the webpack development mode, not in the production mode. Shouldn't that be the same in both cases?

[urugator]

is not easily possible

I don't see why it wouldn't be possible ... just find the place where the array is initialized and instead of [] use observable([], { deep: false })

development mode, not in the production mode

https://github.com/mobxjs/mobx/blob/6c94f9409b489a455fdf4b20310fea8be8e8cdbf/src/v4/core/derivation.ts#L160

The change in behavior is probably caused by https://github.com/mobxjs/mobx/pull/2412

[danrot]

is not easily possible

I don't see why it wouldn't be possible ... just find the place where the array is initialized and instead of [] use observable([], { deep: false })

Okay, maybe I phrased it wrong :see_no_evil: The thing is that this is an extension point in our application, and similar code could be written in other places. Therefore I would prefer if it work when being used with standard array as well.

But solving it with toJS in the other place would case a deep copy and therefore might have a performance impact, right?

development mode, not in the production mode

https://github.com/mobxjs/mobx/blob/6c94f9409b489a455fdf4b20310fea8be8e8cdbf/src/v4/core/derivation.ts#L160

Good to know! So the enforceActions configuration does only have an impact in the development mode anyway?

The change in behavior is probably caused by #2412

Thank you for the information :slightly_smiling_face:

[urugator]

Besides restructuring your app (expose methods instead of mutable data) I can suggest to store callbacks instead of elements () => <div/>

[danrot]

We are developing a product, and both of your suggestions would be breaking changes, so we can't really do that right... But going to keep that in mind for a future major release.