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

log warning or error if duplicate instances of mobx are found #1018

Closed
4 of 6 tasks
nyura123 opened this issue May 25, 2017 · 4 comments
Closed
4 of 6 tasks

log warning or error if duplicate instances of mobx are found #1018

nyura123 opened this issue May 25, 2017 · 4 comments

Comments

@nyura123
Copy link

nyura123 commented May 25, 2017

I have a:

  1. Question

  2. Issue:

  3. Idea:
    Log warning or error if duplicate instances of mobx are found.

    • What problem would it solve?
      Save hours on debugging problems that happen when there are duplicate instances of mobx.
      My app and a dependent lib both use mobx. When I npm link'd the lib, it resulted in a second instance of mobx being used, causing the app's mobx observables not to be updated when expected.
    • Will others benefit from this change as well?
      I think so as there are multiple scenarios when duplicate instances of mobx can occur - e.g. Weird bug since 3.x #763, possibly Dynamically imported module in react won't react to Mobx observable #966, Breaks mobx danielearwicker/bidi-mobx#4
    • Are you willing to (attempt) a PR yourself?
      Yes. One idea is to set either a window.__mobxIsFound or global.__mobxIsFound at the top of mobx lib, and just print a console.error/warning if the flag is already true.

Just wanted to bring this up as an idea to see if others might find it useful.

@mweststrate
Copy link
Member

Ideas welcome! Note that there are 4 different situations

  1. MobX is purposefully isolated from other instances. E.g. two packages use (a different version of) mobx internally and those should not interfere
  2. MobX is packaged in other packages, but they should still share state. That is where shareGlobalState comes into play. Not sure if this is actually used anywhere, it exists basically for backward compatibility with MobX 2 where this was the default.
  3. Multiple MobX instances are unintentionally loaded, because of linking, using dep instead of peer dep etc. etc. which is your situation (in MobX 2 this didn't cause real trouble because of 2.).
  4. Multiple packages use mobx and intend to share it. They specify mobx as peer dependency. This is the proper solution for scenario 2. (And used in mobx-react, mobx-state-tree etc).

I think we can quite safely deprecate option 2, and don't think about it too much. Option 4 is fine with regards to this idea, as it will result in only a single mobx instance. The trouble then is; how to distinguish situation 1 from situation 3?

@nyura123
Copy link
Author

Thanks, I didn't think about 1 (where you might actually want isolation/multiple instances).

For distinguishing between 1 vs 3, maybe it should be up to libs to tell mobx whether to warn of duplicate instances. Or just always logging "mobx lib loading" might be enough of a hint for debugging purposes, by making multiple instances more obvious.

For now I will try comparing my lib's mobx === client's mobx.

thanks!

@Baael
Copy link

Baael commented Jun 2, 2017

So basically we have to do

import {  Atom, Reaction, extras } from 'mobx'
extras.shareGlobalState()

and

import {  observable, extras  } from 'mobx'
import myFancyFunction from 'mobx-fancy-function'
extras.shareGlobalState()

Is it possible in future mobx versions to share state in some namespaces instead of only in global?
Like:

extras.shareState('react', 'router') // internal namespaces, per functionality
extras.shareState('global') // global namespace, main thread of app
extras.shareState('my-own-namespace') // some customized namespaces, for example per framework

@mweststrate
Copy link
Member

mweststrate commented Jul 7, 2017

Found a way to properly add the warning, by requiring people to also signal if they want to use mobx in isolation. Version 3.2 will warn about this in the future (if all mobx versions are on 3.2+). Thanks for the suggestion @nyura123

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

3 participants