feat: derivationRequiresObservable & observablesRequiresReaction

[Bnaya]

PR checklist:

• Added unit tests
• Updated changelog
• Updated docs (either in the description of this PR as markdown, or as separate PR on the gh-pages branch. Please refer to this PR). For new functionality, at least API.md should be updated
• Added typescript typings
• Verified that there is no significant performance drop (npm run perf)
• Backport to mobx 4 branch (?)

Feel free to ask help with any of these boxes!

The above process doesn't apply to doc updates etc.

[coveralls]

Coverage increased (+0.09%) to 93.381% when pulling b1a30f5 on observables-requires-derivation into 81c1bcd on master.

[danielkcz]

Great stuff indeed. Got myself trapped by both of these caveats couple of times too.

[Bnaya]

I'm not sure how to update the changelog, do we have any script that do that, or manually?

[danielkcz]

I'm not sure how to update the changelog, do we have any script that does that, or manually?

Still manually. And tick other checkboxes as well when completed :)

[danielkcz]

I see that 5.13.1 / 4.13.1 is still unpublished, but considering this is a new feature it should go into 5.14.0 / 4.14.0 and we shall publish before merging this I suppose.

[Bnaya]

I've pushed the changelog update

[urugator]

Change observablesRequiresReaction to observableRequiresReaction
so it's gramatically correct and matches other two

Change derivationRequiresObservable to reactionRequiresObservable
to match the other two

requiresObservable option should be added to reaction/autorun (similary to computed's requiresReaction)

Note on defaults:
observableRequiresReaction can't be true by default, because actions are optional by default and working with observable models outside of reactive context is completely valid (in practice I think it can be useful to turn this on after init logic).

reactionRequiresObservable - while reactions without observables are de-facto an error, being able to slap observer everywhere without thinking about deps is also useful

Could you add some test cases where observables are accesses indirectly (through computed) or inside action (eg inside autorun)?

[Bnaya]

requiresObservable option should be added to reaction/autorun (similary to computed's requiresReaction)

Can you please elaborate?

Note on defaults:
observableRequiresReaction can't be true by default, because actions are optional by default and working with observable models outside of reactive context is completely valid (in practice I think it can be useful to turn this on after init logic).

reactionRequiresObservable - while reactions without observables are de-facto an error, being able to slap observer everywhere without thinking about deps is also useful

I personally think it's not a good practice, and the developer should be aware to what gonna render and why.
And when you are working in environment of mixed react & mobx state it can get really messy.
I don't want "dumb" components to be wrapped with observer
If i could do that in the type system/static analysis, i would totally do.
But i'm really OK with keeping them both optional.

Could you add some test cases where observables are accesses indirectly (through computed) or inside action (eg inside autorun)?

I will

[urugator]

Can you please elaborate?

Ability to configure this per reaction - reaction(deriv, effect, { requiresObservable: true })
Related: #1197 (comment)

OT:

the developer should be aware to what gonna render and why

Why not to use explicit subscription then?

[danielkcz]

being able to slap observer everywhere without thinking about deps is also useful

I also don't agree, why would someone want to deoptimize on purpose? I think it comes more from the current lack of validation fixed by this PR that kinda forces you to just "slap observer everywhere". I don't see an actual reason for doing it otherwise.

That said, with requiresObservable on reactions would give that choice which is fine. I would probably enable it by default for observer anyway. And it should fall back to the global setting if there is none specified for a reaction.

[urugator]

The lib is all about automatic subscription management. The idea is that you don't have to worry about what depends on what and you shouldn't experience staleness once you fulfill the contract - observable on data and observer on component.
Making everything observable/observer is an easy to follow and straithforward way to do that.

comes more from the current lack of validation

Agree, but as mentioned the fix is not generally applicable, eg it makes sense only with enforceAction('strict') and I am afraid there may be some false positives.

What's the expected behavior of:

mobx.configure({ observablesRequiresReaction: true })
autorun(() => {
  untracked(() => x.y);
})

Can you add it to tests as well please?

How do you feel about labelling this "experimental" and waiting for some feedback?

[Bnaya]

I prefer to keep it optional and it's totally can be marked experimental so we could tweak the behaviour later without breaking changes.

reaction(deriv, effect, { requiresObservable: true })

We will need to also change mobx-react api to expose also that
should it fail or warn? looking at computedvalue code, i think there's something not optimal there.
on dev time it will fail and on prod time it will just work.
its different behaviour between prod & dev, not just warning on dev
https://github.com/mobxjs/mobx/blob/master/src/core/computedvalue.ts#L270-L289

[urugator]

Ignore on production for performance sake I suppose...
I think that the global configuration only warns (rather failing) so there is some space for intentionally exceptional behavior (some workaround perhaps) without crashing the app.

[Bnaya]

Another thing that came up in my mind:
Add warning when action didn't change any observable value.
But maybe better keep to to another PR?

[urugator]

Add warning when action didn't change any observable value.

The change can be conditional or deferred:

@action doSomething(a) {
  if (this.cond) {
     this.x = a;
  }
}

@action doSomething() {
  // action is untracked to avoid accidental subscription (so it's not entirely useless even if you don't change anything):
  const param = this.y;
  xhr.post(param).then(() => this.x = "a");
}

[Bnaya]

changes pushed
After we will settle this PR, i will make the respective changes to mobx 4 & docs PRs

[Bnaya]

@urugator friendly ping :)
Just in case you've missed it, i know it took me long time

[urugator]

Just to clarify... I didn't mind that it didn't warn inside untracked, I was concerned by the reason why it didn't warn.
It didn't warn because there was a batch, but it shouldn't depend on a presence of batch ... it should depend on the presence of derivation/action - and both derivation/action was missing - so at that time one would expect it to warn (regardless of what the actual behavior should be)... Hope it makes sense.

[urugator]

@mweststrate you may want to take a look at this.

To summarize:
2 new global configuration options:

reactionRequiresObservable: boolean = false

• warns when derivation.observing.length === 0
• also configurable per individual reaction/autorun/when (via requiresObservable option)
• motivation: help the developer to be aware of the reactive parts of the code, help to catch cases when you touch the observable only after if that wasn't true on the first run.
• implemented as simple check inside track function

observableRequiresReaction: boolean = false

• warns when any observable is acccessed outside of derivation or action
• motivation: reveal dereferencing outside of observer (eg passing to 3rd party lib, forgetting observer, dereferencing too soon, etc)
• implemented similary to enforceAction - global flag allowStateReads, switched in start(end)Action/trackDerivedFunction, checked in reportObserved
• unsure about the option name, the most descriptive name would be readingObservableRequiresReactionOrAction, input welcome (enforceReaction, ...??)

Both currently labelled as experimental.

[mweststrate]

Thanks for summarising! Appreciate it :) Both features sound pretty cool for debugging purposes. Didn't review the code, but I think these are good features to have. Naming is hard indeed, but at least there is a cool symmetry :-P.

…

On Tue, Sep 24, 2019 at 5:10 PM urugator ***@***.***> wrote: @mweststrate <https://github.com/mweststrate> you may want to take a look at this. To summarize: 2 new global configuration options: reactionRequiresObservable: boolean = false - warns when derivation.observing.length === 0 - also configurable per individual reaction/autorun/when (via requiresObservable option) - motivation: help the developer to be aware of the reactive parts of the code, help to catch cases when you touch the observable only after if that wasn't true on the first run. - implemented as simple check inside track function observableRequiresReaction: boolean = false - warns when any observable is acccessed outside of derivation or action - motivation: reveal dereferencing outside of observer (eg passing to 3rd party lib, forgetting observer, dereferencing to soon, etc) - implemented similary as enforceAction - global flag allowStateReads, switched in start(end)Action/trackDerivedFunction, checked in reportObserved - unsure about the option name, the most descriptive name would be readingObservableRequiresReactionOrAction, input welcome ( enforceReaction, ...??) Both currently labelled as experimental. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2079?email_source=notifications&email_token=AAN4NBEJORWESCKTRRTUN53QLI3XPA5CNFSM4IMTODJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7O5B5Q#issuecomment-534630646>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAN4NBBLD6ULWVLLOX3HTBLQLI3XPANCNFSM4IMTODJA> .

[Bnaya]

I've updated the mobx 4 backport PR

[mweststrate]

Looking great so far! Left some comments, but nothing big :)

[mweststrate]

Sure, feel free to merge! I'll add a note to self to tinker another time about messaging

…

On Fri, Sep 27, 2019 at 9:58 AM Bnaya Peretz ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/core/derivation.ts <#2079 (comment)>: > @@ -155,12 +160,23 @@ export function checkIfStateModificationsAreAllowed(atom: IAtom) { ) } +export function checkIfStateReadsAreAllowed(observable: IObservable) { + if ( + process.env.NODE_ENV !== "production" && + !globalState.allowStateReads && + globalState.observableRequiresReaction + ) { + console.warn(`[mobx] Observable ${observable.name} being read outside a reactive context`) As these feature marked as experimental, i would prefer to push it forward and get feedback, and then improve developer experience — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2079?email_source=notifications&email_token=AAN4NBG3SBGO3WI6NWR6KV3QLXDJTA5CNFSM4IMTODJKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCGERAAQ#discussion_r328975946>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAN4NBCLKG77JXG5PTCHMWLQLXDJTANCNFSM4IMTODJA> .

[Bnaya]

I've merged the PRS!

What's the policy regarding publishing?
also, i wish we had something that publishes canary versions for each PR or so

[mweststrate]

There is currently no policy :'). Just some ideas to incorporate `np` like done in immer. But I'll try to cut a release somewhere today. TY!

…

On Mon, Sep 30, 2019 at 2:20 PM Bnaya Peretz ***@***.***> wrote: I've merged the PRS! What's the policy regarding publishing? also, i wish we had something that publishes canary versions for each PR or so — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2079?email_source=notifications&email_token=AAN4NBC47N4MHAZBL27TXW3QMH4I7A5CNFSM4IMTODJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD75TMVA#issuecomment-536557140>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAN4NBCPFJVTDSILVFLTN2LQMH4I7ANCNFSM4IMTODJA> .

[mweststrate]

Released!

[Bnaya]

I've created a blog post regarding these features:
https://medium.com/@bnaya/mobx-5-14-0-will-help-you-stay-reactive-and-only-when-you-need-to-2e897d3bfabe
I would love to have some feedback before publishing

[mweststrate]

Looks awesome! I think it is pretty clear. Make sure to link it in the docs! Maybe a link on top of https://mobx.js.org/best/trace.html would be neat as well.

…

On Wed, Oct 2, 2019 at 11:52 PM Bnaya Peretz ***@***.***> wrote: I've created a blog post regarding these features: ***@***.***/mobx-5-14-0-will-help-you-stay-reactive-and-only-when-you-need-to-2e897d3bfabe I would love to have some feedback before publishing — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2079?email_source=notifications&email_token=AAN4NBFWFUO6VS5SBTRBAWDQMUQ3XA5CNFSM4IMTODJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAGN57I#issuecomment-537714429>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAN4NBESUWZ2NIUOCCDA74TQMUQ3XANCNFSM4IMTODJA> .