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

in operator regression for actions #1639

Closed
2 of 3 tasks
getkey opened this issue Jan 14, 2021 · 7 comments
Closed
2 of 3 tasks

in operator regression for actions #1639

getkey opened this issue Jan 14, 2021 · 7 comments

Comments

@getkey
Copy link
Contributor

getkey commented Jan 14, 2021

Bug report

  • I've checked documentation and searched for existing issues
  • I've made sure my project is based on the latest MST version not applicable
  • Fork this code sandbox or another minimal reproduction.

Sandbox link or minimal reproduction code

https://stackblitz.com/edit/in-regression-mobx-state-tree?file=index.html

import { types } from "mobx-state-tree";

const User = types
  .model({
    name: ""
  })
  .actions(self => ({
    setName(name) {
      self.name = name;
    }
  }));

const john = User.create();
console.log("This should be true:", "setName" in john);

Describe the expected behavior

The in operator should detect actions. I've observed that the regression appeared somewhere between version 4.0.2 and 4.0.3.

Describe the observed behavior

The in operator doesn't detect actions on instances of the model.

getkey added a commit to getkey/ble that referenced this issue Jan 14, 2021
This reverts commit 850d002.
There is a regression in mobx-state-tree where `'setIsStatic' in selectedEntity` does not work anymore. See mobxjs/mobx-state-tree#1639
@iChenLei
Copy link
Member

Diff v4.0.2 with v4.0.3

Github code diff link
The main code change in packages/mobx-state-tree/src/utils.ts

- import { isObservableArray, $mobx, getAtom } from "mobx"
+ import { isObservableArray, _getGlobalState, getAtom } from "mobx"

- export const mobxShallow =
-     typeof $mobx === "string" ? { deep: false } : { deep: false, proxy: false }
+ export const mobxShallow = _getGlobalState().useProxies
+    ? { deep: false }
+     : { deep: false, proxy: false }

The $mobx in mobx6 is a Symbol

  export const $mobx = Symbol("mobx administration")

🔥 When we use modern browser which support Proxy 🔥

typeof $mobx === "string" is false in v4.0.2
_getGlobalState().useProxies is true in v4.0.3

v4.0.2 mobxShallow = { deep: false, proxy: false }

v4.0.3 mobxShallow = { deep: false }

Reproduction Example

v4.0.2 The john object is a plain object

v4.0.3 The john object is a Proxy object Proxy MDN doc

Mobx Proxy process code: mobx/packages/mobx/src/types/dynamicobject.ts

    has(target: IIsObservableObject, name: PropertyKey) {
        if (name === $mobx || name === "constructor") return true
        if (__DEV__ && globalState.trackingDerivation)
            warnAboutProxyRequirement(
                "detect new properties using the 'in' operator. Use 'has' from 'mobx' instead."
            )
        const adm = getAdm(target)
        // MWE: should `in` operator be reactive? 
        // If not, below code path will be faster / more memory efficient
        // check performance stats!
        // if (adm.values.get(name as string)) return true
        if (isStringish(name)) return adm.has_(name)
        return (name as any) in target
    }

So when we use v4.0.3 console.log('setName' in john); will print false in console....
Hope my analysis is helpful. @jamonholmgren @getkey

@airhorns
Copy link
Contributor

I think this is an issue for views too, not just actions. I can confirm it only happens for me when proxies are used. Is this maybe a plain mobx bug?

@iChenLei
Copy link
Member

@airhorns @getkey It's existing Mobx6 issue
Mobx Github discussions

@urugator
Copy link
Contributor

It has nothing to do with enumerability, but should be fixed by:
mobxjs/mobx#2641

@urugator
Copy link
Contributor

urugator commented Feb 2, 2021

Should work with MobX 6.1.* (MST > 5.0.1)

@iChenLei
Copy link
Member

iChenLei commented Feb 2, 2021

https://stackblitz.com/edit/in-regression-mobx-state-tree-lng8z9

2021-02-02 19 16 58

Yes, fixed by PR 2641!

@getkey
Copy link
Contributor Author

getkey commented Feb 2, 2021

Awesome, thanks @urugator! Closing this issue.

@getkey getkey closed this as completed Feb 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants