Change in behaviour in mobx v5 and v6

[samcooke98]

Intended outcome:

In MobX v5, the following code would cause a crash in both the production and development build variants.

In MobX v6, this is no longer the case.

// v5 
import * as mobx from "mobx/lib/mobx.min.js";
// v6
// import * as mobx from "mobx/dist/mobx.cjs.production.min.js";


mobx.configure({ enforceActions: "observed" });

const store = mobx.observable({ count: 0 });
mobx.reaction(
  () => store.count,
  (val) => console.log("Store is: " + val)
);

mobx.runInAction(() => {
  store.count++;
});
// works;

store.count++;
console.log(store.count);

Codesandbox (v6): https://codesandbox.io/p/sandbox/zealous-volhard-zfm5tj
Codesandbox (v5): https://codesandbox.io/p/devbox/stupefied-fog-527m2r?workspaceId=ws_uqh2WMCajD26pqSUXVFEd

While breaking-changes are to be expected between major versions, I can't seem to find any GH issue, notes in the changelog, or PRs, that explain if this behaviour was or wasn't intentional.

The only reference I can find is #1758 which suggests to leave it up to consumers of the library to decide.

Actual outcome:

I expect that, if strict mode is enabled, for an error to be thrown regardless of the minified vs development

v5(development):

V5 (production):

V6 (production):

V6 (development):

NB: Is it a bug that the value is modified despite the error message implying it's not?

How to reproduce the issue:

Codesandbox (v6): https://codesandbox.io/p/sandbox/zealous-volhard-zfm5tj
Codesandbox (v5): https://codesandbox.io/p/devbox/stupefied-fog-527m2r?workspaceId=ws_uqh2WMCajD26pqSUXVFEd

---

I'm willing to open a PR for this. I understand that changing this behaviour back to v5-behaviour is likely to need to be done in a major release.

[samcooke98]

Looks like this was changed in f60c913

[mweststrate]

V6 is five years old, my memory doesn't go back that far to be able to tell whether that was an intentional change or not at the time (it probably was for good reasons, since it deliberately changed). Releasing a new major has a lot of complications and the benefits are unclear here. Why are you depending on throwing exceptions in production?

[devtempmac]

idk throwing errors and crashing at runtime seems to be the worst behavior -- you get to know about it when client gets to run THAT specific part of the code.
And when your code shipped, in some deployments (eg. on-site no-internet places like banks), you simply cannot re-deploy without going though paperwork and wasting some days

giving out warnings on linting stage is the best option, but the current 'give only visible warnings' isn't a bad choice.

modern dev tooling handles this using telemetry collection, not explode-on-prod

(this isn't even a bug -- rather, it's an improvement to modern dev standards (dont-explode-on-prod) for mobx)

[samcooke98]

Totally fair Michel.

I do think it's confusing behaviour that that warning says "Since strict-mode is enabled, changing (observed) observable values without using an action is not allowed. Tried to modify: ObservableObject@1.count" - but that the application continues working as expected. IMO it makes "strict mode" not really that strict?

We've historically used this strict mode/enforceActions: 'observed', to encourage engineers to wrap their code in action handlers at the 'highest' level, to ensure that all mobx mutations can be appropriately grouped.

I filed this bug-report, after at Canva, we had an issue where:

• An engineer made a change to some promises in one of our applications, forgetting to wrap the .then in action.

  • This engineer chose to rely on CI, rather than local tests. This is understandable in my opinion, as his change was one or two lines.
  • This change passed CI, and was merged into main. Our CI checks would have caught this, if the error occurred in a "production" build.

• Then some other engineers tried to run the same application after pulling main. They would try to load the page, and it would fail / error.

  • I realise after typing this up, it failed because we have manually patched the warning to be a die in development mode. We could remove this custom patch, and go back to a warning, like the upstream mobx. However, we've historically used strict mode to ensure / encourage appropriate grouping of actions. We also try to avoid warnings, as we have hundreds of engineers working in the application, and warnings can quickly become a 'tragedy of the commmons' where lots of engineers introduce warnings, but do not fix them.

[samcooke98]

Actually a thought- What if we made this a new option? Super-strict?

[mweststrate]

Sorry wanted to provide some additional background, but forgot to circle back (currently on holiday, so might need another ping in two weeks ;)), but yeah that might work!