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

[Mobx >= 4] Object.hasOwnProperty fails #1526

Closed
rennerDa opened this issue Apr 30, 2018 · 10 comments
Closed

[Mobx >= 4] Object.hasOwnProperty fails #1526

rennerDa opened this issue Apr 30, 2018 · 10 comments

Comments

@rennerDa
Copy link

rennerDa commented Apr 30, 2018

I don't know if this is a bug in mobx or babel or anything else.
I tested this with MobX 3.6 and earlier versions and it worked. It breaks with Mobx >= v4.

I uses this repository (https://github.com/mobxjs/mobx-react-boilerplate) and changed the following files:
.babelrc

{
  "presets": [
    "es2015",
    "react",
    "stage-0"
  ],
  "plugins": ["transform-decorators-legacy",
      "transform-class-properties",
      "transform-runtime"]
}

package.json

{
  "name": "mobx-react-boilerplate",
  "version": "1.0.0",
  "description": "Boilerplate for MobX + React project with JSX, ES6 and decorator compilation",
  "scripts": {
    "start": "webpack-dev-server --hot --open",
    "lint": "eslint src"
  },
  "repository": {
    "type": "git",
    "url": "https://github.com/mobxjs/mobx-react-boilerplate.git"
  },
  "keywords": [
    "react",
    "reactjs",
    "boilerplate",
    "mobx",
    "starter-kit"
  ],
  "author": "Michel Weststrate <mweststrate@gmail.com> (http://github.com/mweststrate)",
  "license": "MIT",
  "bugs": {
    "url": "https://github.com/mobxjs/mobx/issues"
  },
  "homepage": "http://mobxjs.github.com/mobx",
  "devDependencies": {
    "babel": "^6.23.0",
    "babel-core": "^6.26.3",
    "babel-eslint": "^8.2.1",
    "babel-jest": "^22.4.3",
    "babel-loader": "^7.1.4",
    "babel-plugin-react-intl": "^2.4.0",
    "babel-plugin-transform-class-properties": "^6.24.1",
    "babel-plugin-transform-decorators-legacy": "^1.3.4",
    "babel-plugin-transform-runtime": "^6.23.0",
    "babel-polyfill": "^6.26.0",
    "babel-preset-es2015": "^6.24.1",
    "babel-preset-react": "^6.24.1",
    "babel-preset-stage-0": "^6.24.1",
    "babel-preset-stage-3": "^6.24.1",
    "babel-runtime": "^6.26.0",
    "webpack": "^3.5.5",
    "webpack-dev-server": "^2.7.1",
    "mobx-react-devtools": "^5.0.1"
  },
  "dependencies": {
    "mobx": "^4.2.0",
    "mobx-utils": "^4.0.1",
    "mobx-react": "^5.0.0",
    "react": "^15.1.0",
    "react-dom": "^15.1.0"
  }
}

When I try to check if the Model has a property with "hasOwnProperty" the result is false (if nothing was assigned before):
TodoModel.js

import { observable } from "mobx";

export default class TodoModel {
  id = Math.random();
  @observable title;
  @observable finished = false;

  constructor(title) {
    //this.title = title;
    console.log(Object.prototype.hasOwnProperty.call(this, "title"));
  }
}

If I remove the comment and assign title in front (this.title = title;), hasOwnProperty is true.
This is maybe a bad example, but it shows my problem.

@mweststrate
Copy link
Member

mweststrate commented May 3, 2018

@rennerDa although weird, this behavior is expected and cannot be robustly fixed; @observable (and all other decorators are always applied to the prototype, not to the instances. So you would actually need to ask about the prototype whether it has the property.

What MobX does is moving the property to the own instance as soon as it is read or written for the first time. That is the best we can do with the current implementation of the decorators transformer (the stage 2 proposel fixes this issue).

The reason it probably worked in 3.6 is the fact that finished is initialized, which caused the title to be initialized as well, I'll check whether I can restore that behavior, but conceptually this will remain an issue until the stage 2 proposal implementation lands in babel

@mweststrate
Copy link
Member

I just run the above test against MobX 3.6, and it yields the same results. There might be a subtle difference, but the above example doesn't expose it at least. In MobX 3.6 the log returns false as well. So a more accurate test case is needed to expose a difference

@mweststrate
Copy link
Member

(added cannot reproduce; the issue can be reproduced, but cannot reproduce that it changed from MobX 3)

@rennerDa
Copy link
Author

rennerDa commented May 3, 2018

Thanks for you fast reply. You are absolutely right (sorry, should've tested more). I had to extend the TodoModel a little bit to get it working with Mobx 3.x:
Tested with:

"mobx": "^3.6.2",
"mobx-react": "^4.4.1",

This works in 3 but not with mobx 4.2.0:

import { observable, action } from "mobx";

export default class TodoModel {
  id = Math.random();
  @observable title;
  @observable finished = false;

  constructor(title) {
    //this.title = title;
    this.setTest();
    // console.log(Object.keys(this));
    // console.log(Object.prototype.hasOwnProperty.call(this, "title"));
  }

  @action
  setTest = () => {
    console.log(Object.keys(this));
    console.log(Object.prototype.hasOwnProperty.call(this, "title"));
  };
}

It works because of the setTest property (action). I think, because it is a property, you assign the observables and actions together in mobx 3, but not in mobx >= 4 (it seems so).
If I use a class method it is not working (in v3 and in v4):

@action
  setTest() {
    console.log(Object.keys(this));
    console.log(Object.prototype.hasOwnProperty.call(this, "title"));
  }

Did you change the way actions and observables were assigned to objects? Thanks in advance for every hint/help :) .

@mweststrate
Copy link
Member

Did you change the way actions and observables were assigned to objects

Yes, some stuff changed regarding the initialization of decorators to make it more consistent across platforms. The effect might be that the semantics have become less optimal in certain cases.

The general way to avoid this issue is to read the properties first before reflecting on them (reading / writing will actually cause the properties to move from the prototype to the instance)

Alternatively, since this is mainly a decorator issue (decorators in stage-0 cannot attach behavior to instances, only to prototypes, that is where the lazy behavior originates from), the work around is to use extendObservable in the constructor, which has none of these lazy semantics and should initialize stuff immediately.

@aaronatmycujoo
Copy link

aaronatmycujoo commented Oct 24, 2018

I am also running into this on 3.6.2 and 4.0.0. hasOwnProperty fails to check if an index is out of bounds for an array. Below is the reproducible example and here's the jsfiddle of the example: https://jsfiddle.net/u46da7hb/1/

const { observable, toJS } = mobx

const m = observable({
	state: 'fulfilled',
	value: ['a','b','c'],
})

const n = {
	state: 'fulfilled',
	value: ['a','b','c']
}

console.table({
	mobx: _.has(m.value, 2), // false
	toJS: _.has(toJS(m.value), 2), // true
	native: _.has(n.value, 2), // true
	['mobx hasOwnProperty']: n.value.hasOwnProperty(2), // false
	['native hasOwnPropety']: m.value.hasOwnProperty(2), // true
})

Is this the expected behavior? As you can see, my fix is to wrap the object in toJS which will correctly see if the index is out of bounds

@mweststrate
Copy link
Member

Yes, the behavior is as expected @aaronatmycujoo, and cannot be changed without proxies (that is Mobx < 5)

@mweststrate
Copy link
Member

Also, this thread if about objects. Arrays are different data structures, for follow up questions, please open a separate issue.

@mweststrate
Copy link
Member

Closing for now, will be reopened once #1928 happens.

@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

3 participants