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

computed with ifavailable cause Error #2619

Closed
doxiaodong opened this issue Nov 13, 2020 · 9 comments
Closed

computed with ifavailable cause Error #2619

doxiaodong opened this issue Nov 13, 2020 · 9 comments
Labels
🎁 mobx Issue or PR related to mobx package

Comments

@doxiaodong
Copy link
Contributor

doxiaodong commented Nov 13, 2020

Intended outcome:
cause by Reflect.ownKeys
it is seem broken with JSON.stringify

Actual outcome:
image

How to reproduce the issue:
https://codesandbox.io/s/multi-selection-forked-vdjqn?file=/src/index.tsx

Versions

mobx@6.0.4
useProxies: 'ifavailable'

@danielkcz danielkcz added 🎁 mobx Issue or PR related to mobx package 👓 needs investigation labels Nov 13, 2020
@urugator
Copy link
Collaborator

urugator commented Nov 13, 2020

Minimal repro:

const o = observable({});
autorun(() => {
  JSON.stringify(o);
})

Possible solutions:
a) Don't warn
b) Force user to provide own toJSON or to use other means for conversion
c) When proxies are not available, add default toJSON to every observable object

@urugator
Copy link
Collaborator

Btw toJS has the same problem

@mweststrate
Copy link
Member

This behavior is correct, as JSON.stringify would behave differently indeed a non proxy environment, and this error is an early warning for it. toJS should ideally not suffer from the same thing as it could default to using mobx own iteration mechanism. Feel free to file a separate issue for that.

@doxiaodong
Copy link
Contributor Author

But same code work fine with mobx@5

@urugator
Copy link
Collaborator

urugator commented Feb 10, 2021

Mobx5 required proxy support - it would not work at all in env without proxies, you had to use Mobx4 instead.
Mobx6 lets you decide - by configure({ useProxies: "ifavailable" }), you're saying - this code should work in any environment, but if available use proxies as internal implementation. So mobx will warn anytime your code would break in non-proxy env.

@doxiaodong
Copy link
Contributor Author

Yes, Mobx4 and Mobx5 is fine with JSON.stringify , but Mobx6 with configure({ useProxies: "ifavailable" }) is not. That means it's a breaking. By the way, I think it's better to throw warn for all cases of useProxies

@mweststrate
Copy link
Member

It's breaking because it would break if you would run this code on Internet Explorer, just as this would break on Internet Explorer if you were running this very same code with MobX5 on Internet Explorer. Please read the conversation again, it errors because you configured it to error as an early warning this will not run on older browsers. If you want the MobX 4 behavior, put it on useProxies: "never".

@urugator
Copy link
Collaborator

@doxiaodong In Mobx4 JSON.stringify may not work correctly - if you add or delete prop from the object it would NOT notify the computed. We don't warn in Mobx4, because the warning only works when proxies are available and Mobx4 does not have a proxy based implementation at all - so we simply don't have a way to warn in Mobx4 - but the code is still potentially broken (depends on usage).

@doxiaodong
Copy link
Contributor Author

Thanks,I use always for proxy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎁 mobx Issue or PR related to mobx package
Projects
None yet
Development

No branches or pull requests

4 participants