Mobx: Create better error for missing decorated getters

[jefffriesen]

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

[mweststrate]

Computed properties should not be deleted, as it only removes the descriptor, but the computed value is still there, potentially leaking memory. (I think as a rule of thumb delete should never used in JavaScript, but that is a different point).

The simple prevention for this is to mark the properties as non-configurable. Downside of that is that stubbing will be harder, but it might fix some other errors.

[mweststrate]

Fixed and released in 4.9.0 / 5.9.0

[jefffriesen]

@mweststrate
I looked at your commit. I'm not familiar with the internals of Mobx or the implication of configurable: false.

Your comment about delete in javascript makes me wonder if I didn't get my point across very well. I wasn't talking about programmatically deleting a computed property - I was just saying deleting it manually while refactoring.

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,  // Forget to remove this and the error is hard to know what happened
  tick: action
});

Maybe your fix did address this, but I just wanted to clarify because I didn't understand your response. But if you feel like this is addressed, great. Thanks

[mweststrate]

Oh sorry! I understood it as programmatically deleting :) Reopeneing.

Op di 22 jan. 2019 om 16:06 schreef Jeff Friesen [e-mail]:

@mweststrate https://github.com/mweststrate
I looked at your commit. I'm not familiar with the internals of Mobx or
the implication of configurable: false.

Your comment about delete in javascript makes me wonder if I didn't get
my point across very well. I wasn't talking about programmatically deleting
a computed property - I was just saying deleting it manually while
refactoring.

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, // Forget to remove this and the error is hard to know what happened
tick: action
});

Maybe your fix did address this, but I just wanted to clarify because I
didn't understand your response. But if you feel like this is addressed,
great. 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-456432378, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABvGhDPuQMBmea9buPiT7YEuaD74bxASks5vFyjZgaJpZM4Z9ZJs
.

[Android3000]

@mweststrate Just wanted to mention as well, as you called out in your previous post, I noticed when this change went in that a few of my tests started breaking. I had some unit tests taking advantage of Jest's spyOn to simplify a few computed properties, and indeed these started erroring out now due to the computed props now being non-configurable. Not a huge deal as it was only a couple of tests, but it was kinda nice to be able stub.

Thanks for all the work on MobX!

[mweststrate]

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:

class X {
  @computed getMyFummies() { return this._getMyFummies() } 

_getMyFummies() { 
   /* actual logic here */ 
  }

That makes it both possible to inherit _getMyFummies and spying it.

[Android3000]

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

[mweststrate]

Correct

Op di 22 jan. 2019 17:37 schreef Android3000 [e-mail]:

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
.

[alendit]

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

[zetoke]

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?

[evoyy]

https://github.com/mobxjs/mobx/commit/1b0f5ed4b24844498152e90c08c6a3287fe468d5 also broke some of my tests as I can no longer stub computed properties with sinon. Exception thrown: TypeError: Cannot redefine property. The suggested workaround works fine for me.

[mweststrate]

@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 }) ?

[r3nya]

Hey @mweststrate!

'test only' setting. Something like mobx.configure({ enableStubbing: true }) ?

I guess it will work!

[zetoke]

@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.

[jpalomar]

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 })

[asaarnak]

Any news on this?
We currently use alot Object.defineProperty or jest.spyOn in tests and https://github.com/mobxjs/mobx/commit/1b0f5ed4b24844498152e90c08c6a3287fe468d5 prevents updating mobx.

[mweststrate]

PR is welcome :)

[goldylucks]

@mweststrate is it a good first task for a new maintainer? If so, I'll take a swing at it with some guidance :)

[FredyC]

@goldylucks The label "ready-to-be-implemented-by-volunteer" is exactly for that :) Probably best if you can try your best first and then we can try to help you out. Don't be afraid to create a draft PR to show your attempts.

[goldylucks]

challenge accepted!

[goldylucks]

@FredyC ,
I started poking around the relevant bits of code, and I see that it's already configurable and tested.

What am I missing here?

It might be completely obvious, I'm new to the internals of mobx

[FredyC]

To be honest, I have no idea either :) I just briefly skimmed through here, apparently, there is some issue when interacting with mobx-state-tree. Perhaps you can start by creating a reproduction in CodeSandbox if that helps to discover the problem. It will allow you to switch between versions easily and see what brakes.

[goldylucks]

ok, if someone can provide some guidance I will look into it. I also haven't used MST

[wlindner]

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

[andreipfeiffer]

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

[guerjon]

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.

[FredyC]

PR with a fix has been merged, but not released yet. Closing the issue.

[lock[bot]]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs or questions.

[lock[bot]]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs or questions.