Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support @action on constructors #338

Closed
Haemoglobin opened this issue Jun 17, 2016 · 21 comments
Closed

support @action on constructors #338

Haemoglobin opened this issue Jun 17, 2016 · 21 comments

Comments

@Haemoglobin
Copy link

Haemoglobin commented Jun 17, 2016

Hi,

I get an error when I try to assign a value to an observable property in a class constructor and have useStrict enabled. Seems odd to wrap those assignments within a runInAction block within the constructor because being a object in the middle of being constructed, nothing should be observing it yet?
It also creates a tonne of debug messages as all the records are constructed which I don't really want because I'm not mutating at this point, just setting up new observable objects.

By the way, absolutely amazing looking library by the way, currently converting my code base over from redux and deleting tonnes of code in the process and it's looking far simpler (especially due to the awkward code you get when you try to combine typescript redux and immutabilty)

Thanks!
Hamish

@mrcrlee
Copy link

mrcrlee commented Jun 19, 2016

Whether you are using runInAction or creating an action method to alter the observable properties, I think consistency is important to the declarative nature of mobx. In that case, specifying to everyone that each method which can alter state is declared as such is the intent of useStrict. I don't think treating it as 'strict except when.. constructor..' is a path worth going down.

@Haemoglobin
Copy link
Author

Haemoglobin commented Jun 20, 2016

The problem is, you can't decorate a constructor with @action (in typescript at least), so you need to use runInAction inside of it which might be inconsistent with your preference to use the decorator elsewhere in your application.

I have since restructured my application slightly, so before I had a pattern where a service would make the http request and transform the response into the observable objects before returning them to the calling action to attach them to the state. Now I've changed it so the services just return the raw json structures and I convert them to the state objects within the action function so I don't need to worry about doing anything in the constructor as it's now being executed from within an enclosed action. Seems to be the cleanest option given the constraint.

I agree when you say that anything that alters state should be marked with action, the only thing is creating a new object does not alter any state (I.e including setting observable properties in its constructor function), as state is only affected when you attach this object to your existing observable state tree by assigning it to an observed property or pushing it into an observable array etc (or observing it directly after it has been constructed)

Anyway, its not a show stopper for me, I can live with creating these objects from action function callers if it's cleaner for it to remain as is.

Thanks

@mweststrate
Copy link
Member

Note that usually you can assign a value directly when a field is initiated instead of using the constructor, or invoke an action from the constructor, to update values.

That being said it makes sense to be allowed to modify own state in a constructor function. I am not entirely sure whether it is possible to automatically detect that, but otherwise it would be nice if at least @action could be used on constructors as well :)

@mweststrate mweststrate changed the title useStrict and constructors support @action on constructors Jun 20, 2016
@jeffling
Copy link

jeffling commented Jun 24, 2016

That being said it makes sense to be allowed to modify own state in a constructor function.

I could use a general 'constructor exemption from rules' thing for a similar problem where I was transforming one from one type to another in a @computed.

@mweststrate
Copy link
Member

Not sure yet what is the best approach here; decorators are not supported by any spec on constructor functions. Just FYI, I noticed that decorating the class itself with @action (a class is a func after all) seems to work in typescript and mobx 2.3.4, but that is not a real solution. Note that the simplest work around is is to invoke private actions from within the constructor.

@jeffling good point!

@testerez
Copy link

I agree, when constructing objects, I don't need to know that observable properties are being updated. For an example of noisy log: https://testerez.github.io/react-ts/
(Activate Mobx log and click on the smiley)

@panjiesw
Copy link

Hi,
I don't mind about having to invoke private @action decorated function or just using runInAction in constructor, but I do mind about seeing bunch of noises I don't need in the devtools, just like @testerez mentioned above. Imagine having to construct list of instance from JSON received from some API server.

Is there any approaches to suppress the devtool to log state mutation in a constructor?
Should I create an issue in the react devtool repo?

@Haemoglobin
Copy link
Author

Yep, agree with logging noise issue, I do exactly that, populate a grid of rows with 100's/1000's records from the API, each causing mobx to log the state mutations from the constructor when I construct the objects for each row. Makes it harder to find the more useful logs.

@mattruby
Copy link
Member

Not sure if this will help, but in chrome dev tools you can filter console
messages:
https://developers.google.com/web/tools/chrome-devtools/debug/console/console-ui?hl=en#filtering-the-console-output

On Tue, Aug 16, 2016 at 10:45 AM, Haemoglobin notifications@github.com
wrote:

Yep, agree with logging noise issue, I do exactly that, populate a grid of
rows with 100's/1000's records from the API, each causing mobx to log the
state mutations from the constructor when I construct the objects for each
row. Makes it harder to find the more useful logs.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#338 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAIrcuJEy-gBYgzKbPTNrxtHppYJA3rbks5qgdshgaJpZM4I4SfX
.

-Matt Ruby-
mattruby@gmail.com

@panjiesw
Copy link

panjiesw commented Aug 16, 2016

thanks for the reminder of chrome console filter @mattruby !
I'll use that for the time being.
Still, I'd still prefer mobx builtin feature for this, if possible 😃

@pastelsky
Copy link

pastelsky commented Dec 26, 2016

Just hit the same issue. I'm trying to rehydrate the stores on the client side in my isomorphic app, and the constructor seems to be the best place to do so. Unfortunately, strict mode doesn't allow me to do so directly.

export default class DataStore {
  constructor(initialState = {}) {
    extendObservable(this, {
      data1: [],
      data2:[],
    }, initialState)
  }

// Hydration of stores
export default function getStores(initialState = {}) {
  const hydratedStores = {}

  Object.keys(stores).forEach((value) => {
    hydratedStores[value] = new stores[value](initialState[value])
  })

  return hydratedStores
}

@mweststrate
Copy link
Member

This issue has become obsolete by MobX 3.1, where even in strict mode it is allowed to update not-yet observed observables, fixing this issue around constructors :)

eugenkiss added a commit to eugenkiss/static-mobx-routing that referenced this issue Mar 17, 2017
Needed to upgrade mobx to 3.1 as well because of the following issue: mobxjs/mobx#338
Since `new PostRoute(...)` was being called in a render method and its constructor set an observable property.
@josh-endries
Copy link

I just ran into this in mobx 4.2.0 :/

@pelotom
Copy link

pelotom commented Jun 10, 2018

This code:

import { configure, observable } from 'mobx';

configure({
  enforceActions: "strict"
});

export class Foo {
  @observable x = "";
}

new Foo();

works fine when transpiled by Babel, but fails with a strict mode violation in TypeScript. Which is the correct behavior?

@xeger
Copy link

xeger commented Oct 10, 2018

I'm seeing the same behavior as @pelotom FWIW: TypeScript seems to hoist field-declare-and-initialize statements into the constructor, causing them to throw an error when using MobX strict mode.

Did behavior deliberately revert for MobX 4, @mweststrate?

Conceptually speaking, a clean fix would be for the mutation-guard code to allow the first mutation ever to the observable, even in strict mode, unless it is already being observed -- but to disallow any further mutations (whether observed or not) after the first one.

I'm not sure whether that solution would add too much overhead to the observable guts or, or whether it's consistent with MobX ideology. If there's support for it, perhaps I could have a stab at implementing.

@hlibco
Copy link

hlibco commented Oct 12, 2018

I had exactly the same problem mentioned by @pelotom and @xeger when using TypeScript.

"mobx": "5.5.0",
"mobx-react": "5.2.8",
"ts-node": "7.0.0",
"typescript": "3.0.3"

@mweststrate
Copy link
Member

The issues above are hard to work around, as it is sensitive to some language transformations. But the rule of thumb is the following:

Every event in your system should be an action

If you follow that rule, you should never run into the issue above, as every object mutation and creation should in the end originate from an event (user clicked something, data was received from server, websocket pushed a message etc etc). The sole exception to that is your application initialization code, but in a well organized application that doesn't rely on global module state, this means that you have only one (or a few) places where you need to wrap an additional runInAction and construct your stores in there.

(I consider export default new Store() an antipattern, but if you insist, export default runInAciton(() => new Store()) should do)

@muhajirdev
Copy link

Hi @mweststrate . I am quite new to mobx. I am interested to read more about export default new Store() as anti pattern. Do you know any writings / blog that I read further to understand why it is an anti pattern?

@mweststrate
Copy link
Member

@muhajirframe not from top of my head, and not related to mobx, but it introduces global (module) state; meaning that for example in unit tests you will have to reset your stores.

@marianban
Copy link

One possible way how to overcome this issue in typescript with strict property initialization enabled is to use a private init function in combination with a definite assignment assertion (!):

export class Store {
  @observable
  public value!: string;

  constructor() {
    this.init();
  }

  @action
  private init() {
    this.value = 'default value';
  }
}

@lock
Copy link

lock bot commented Jul 21, 2019

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 lock bot locked as resolved and limited conversation to collaborators Jul 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests