mobx: Create better error for missing decorated getters

In the case of using the decorate syntax (in an environment where decorators aren’t enabled), there is a really confusing error message.

For example, in the documentation on how to not use decorators, you have this example:

class Timer {
  start = Date.now();
  current = Date.now();

  get elapsedTime() {
    return this.current - this.start + "milliseconds";
  }

  tick() {
    this.current = Date.now();
  }
}
decorate(Timer, {
  start: observable,
  current: observable,
  elapsedTime: computed,
  tick: action
});

If you delete the get elapsedTime(){ function, you get this error:

Uncaught TypeError: Cannot read property 'get' of undefined
    at Object.propertyCreator (mobx.module.js:596)
    at initializeInstance$$1 (mobx.module.js:373)
    at MobxStore.set [as activeFile] (mobx.module.js:358)
    at new MobxStore (MobxStore.js:33)

The problem is that the elapsedTime: computed property doesn’t get deleted with the function, which is easy to do when they aren’t co-located (without decorators). This keeps happening to me and I’ve memorized the error, but the first couple times where hard to track down. It seems like there is no intentional error checking - it just errors down the stack at instantiation of the class.

I think it should error (give the dev a chance to clean up cruft) but give them more information why it’s throwing an error.

Thank you

About this issue

Original URL
State: closed
Created 5 years ago
Reactions: 2
Comments: 29 (10 by maintainers)

Commits related to this issue

made computed props non-configurable to earlier detect delete or inheritance abuse. Fixes #1867 — committed to mobxjs/mobx by mweststrate 5 years ago
#1867 Add warning for missing decorated getter — committed to berrunder/mobx by berrunder 4 years ago
Report better error for missing decorated getter #1867 (#2319) * #1867 Add warning for missing decorated getter * Update changelog * Fix: move changelog line * Backport to V4 — committed to mobxjs/mobx by berrunder 4 years ago

Most upvoted comments

If you get the property is not declared configurable error when using Jest spyOn on mobx @computed properties, you can add this:

import { configure } from "mobx";
configure({ computedConfigurable: true });

Thanks @wlindner 😉

PS: it’s documented here https://mobx.js.org/refguide/api.html

andreipfeiffer on Aug 7, 2019

@zetoke, I don’t like settings in at all in generally, but it might actually be a nice idea in this case, to have it a ‘test only’ setting. Something like mobx.configure({ enableStubbing: true }) ?

mweststrate on Feb 14, 2019

Yes, that is precisely why we keep configurable true in general, but in this specific case it can also lead to quite confusing bugs. Not that both spy and inheritance issues can be worked around by extracting the logic like:

That quite breaks the flow of writing tests that we use in a company with spyOn in jest or manually mocking with Object.defineProperty. And this workaround will work but it looks super weird (duplicating a lot of properties/fields) and we also need to rewrite a huge amount of places in current projects. Hmmm, can it be kind of a setting may be?

zetoke on Feb 7, 2019

I was having test failures after upgrading mobx in my app that uses mobx-state-tree. I couldn’t stub getter properties that were mst views after the upgrade. I came across this thread and fixed it by putting this code in my test setup.

const mobx = require('mobx');
mobx.configure({ computedConfigurable: true });

Then I could stub out getter views this way.

sinon.stub(newBlock, 'hash').get(() => '000');

Which I got from sinon’s docs https://sinonjs.org/releases/latest/stubs/#stubgetgetterfn

refactornator on Jul 14, 2019

@mweststrate Yeah, I think so. It looks perfect!

Agree with you, that usually it’s not a good choice to add a new setting in general 😄 But here, I don’t see an other way.

zetoke on Feb 14, 2019

Hi @andreipfeiffer yes, didn’t realize that my setup test is call enzume-setup.ts, that is been set in setupTestFrameworkScriptFile in the jest config file. That works thank you.

guerjon on Dec 16, 2019

would it be fair to consider this revision: https://github.com/mobxjs/mobx/commit/f0473c83c75930406bc91699f6b9013d118b5c1e#diff-147a1f3618d08f1399ce092b72872853R328 a breaking change?

could we release a5.9.1 \ 4.9.3 version that reverts this back? and release a new feature for 5.10.0 \ 4.10.0 that includes the feature to configure Mobx with mobx.configure({ canStub: true })

jpalomar on Mar 3, 2019

1b0f5ed4b24844498152e90c08c6a3287fe468d5 breaks latest mobx-state-tree, see https://github.com/mobxjs/mobx-state-tree/issues/1143 .

alendit on Jan 22, 2019

Correct

Op di 22 jan. 2019 17:37 schreef Android3000 notifications@github.com:

Oh nice – if the observed properties involved in the computation are within this._getMyFummies(), the computed function will still be recomputed when they change? That’s a great workaround - thanks!

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx/issues/1867#issuecomment-456468231, or mute the thread https://github.com/notifications/unsubscribe-auth/ABvGhL6l9Ptt7E0MmtbdtzFmM6_SrdSKks5vFz4ugaJpZM4Z9ZJs .

mweststrate on Jan 22, 2019