afterCreate is firing when a computed accesses a child causing a side effect

[mattruby]

FYI: I've posted this on spectrum. It looks like a possible bug.

Here's a small (50 ln) codesandbox example showing what I'm up against:
https://codesandbox.io/s/94zy6n96jo

My issue is with a tree where a child has an afterCreate action that modifies another child. If I access the child data using a computed I get the following error:
Error: [mobx] Computed values are not allowed to cause side effects by changing observables that are already being observed. Tried to modify: [email protected]

I assumed afterCreate would fire after the object had been created? Can anyone shed some light on when afterCreate and afterAttach really fire?
Thanks!!

[kristian-puccio]

pretty sure I'm having the same issue.
In my case I'm using mobx 4

[k-g-a]

Actually the error is "correct".
When you "pre-select" the first answer from Product's afterCreate - you modify observable selected from mobx action.
When you get "selectedAnswer" (which is mobx computed), during that get your Questions's afterCreate is called and it sets observable selected to another value.
It's like writing:

    get selectedAnswers() {
      return self.questions.reduce((result, question) => {
        if (question.answers.every(a => !a.selected))
          question.answers[0].selected = true
        result.push(question.answers.find(a => a.selected));
        return result;
      }, []);
    }

But I totally agree that such behaviour is not expected and quite unobvious.

This happens due to afterCreate is actually called during observalbe instance creation:

• for roots it happens at the moment you call .create
• for children (i.e. Product.questions or Question.answers / Question.answers[0] / Product.questions[1].answers[2] and so on) - on first access (from within the computed in this case).

First afterAttach happens right after afterCreate.
Subsiquent afterAttach'es happen when instance is moved to another parent within the same tree. Worths mentioning that detach operation extracts instance from tree, effectively setting it's parent to null, thus re-adding detached item will cause afterAttach to fire.

At the moment I can not figure out any way to fix this without breaking change (i.e. async afterCraete or introducing another hook), but i'll take time this week to dig deeper.

[luisherranz]

Oh, so this is derived from the fact that starting from MST3 afterCreate doesn't fire until the instance is observed, right?

Maybe computed values should be allowed to fire afterCreates. After all, afterCreates should only fire once.

[mattruby]

@k-g-a thanks for the great explanation! And thanks for looking into the issue. I'll try to work around this for the time being.

[k-g-a]

Oh, so this is derived from the fact that starting from MST3 afterCreate doesn't fire until the instance is observed, right?

Right. MST3 introduced lazy instances creation.

[kristian-puccio]

Not ideal but I managed to work around it by wrapping the call to the action (inside afterCreate) that does the mutations in a setTimeout. Seems to have fixed the errors

[mattruby]

I ended up working around this by making explicit calls to preconfig actions after creation. Not great, but it will keep me moving.

[mweststrate]

@k-g-a, mobx exposes an internal api, _allowStateChanges that could be used to explicitly allow these kind of side effects. I think we could use this probably in the lazy instantiation code to fix this problem so that this no longer breaks. Not sure yet if this also has potential downsides.

[xaviergonz]

@mweststrate I tried to fix it using that but it will still throw, I think it is because of this:

if (globalState.computationDepth > 0 && hasObservers)
        fail(
            process.env.NODE_ENV !== "production" &&
                `Computed values are not allowed to cause side effects by changing observables that are already being observed. Tried to modify: ${
                    atom.name
                }`
        )

the check that throws the exception completely ignores globalState.allowStateChanges, which is what is set by _allowStateChanges

I guess that if would need to be changed inside mobx to

if (!globalState.allowStateChanges && globalState.computationDepth > 0 && hasObservers)

before it can be fixed?

[xaviergonz]

or maybe creating a whole new globalState.allowStateChangesInsideComputed / _allowStateChangesInsideComputed to be on the safe side

[xaviergonz]

just created a PR on mobx for it

[mweststrate]

@xaviergonz ok sorry, should have read above comments before reading the PR on mobx, lol :). Ok, I'm going to check what the impact of that check would be, adding another global state modifier wrapping function doesn't make it easier to grasp :)

[k-g-a]

@xaviergonz thanks for picking that up! Any update on this?

[xaviergonz]

still waiting on the PR on the mobx side to resolve before this one can be addressed I'm afraid

[mweststrate]

@xaviergonz released! MobX 4.5.0 / 5.5.0 includes the change

[xaviergonz]

@mweststrate and there's the PR :)

[xaviergonz]

should be fixed in 3.5.0, but only if mobx >=4.5.0 || >= 5.5.0 is installed as well

[mattruby]

Thank you!

…

On Sat, Sep 29, 2018, 10:58 AM Javier Gonzalez ***@***.***> wrote: should be fixed in 3.5.0, but only if mobx >=4.5.0 || >= 5.5.0 is installed as well — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#967 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAIrcqaYluKJhgYqw1Z_Qq0GUt37QRcPks5uf5iPgaJpZM4V8jRQ> .