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

Road to 4.0 #1076

Closed
14 tasks
mweststrate opened this issue Jul 4, 2017 · 105 comments
Closed
14 tasks

Road to 4.0 #1076

mweststrate opened this issue Jul 4, 2017 · 105 comments

Comments

@mweststrate
Copy link
Member

mweststrate commented Jul 4, 2017

Hi all!

Time to start thinking about the next major of MobX. These are some of the ideas I have around them. Global goal:

Leverage modern features in evergreen browsers to address current quirks in the MobX api

Quirks being:

  1. Observable arrays not being arrays,
  2. property additions not being observed
  3. Maps only supporting primitive keys and Sets not being present [Mobx 4] Make ObservableMap work with non-string keys (like es6 Map does)  #800

Tools to address the quirks: levering proxies #776 and native map implementations #800

Breaking changes

Also fix #940 if not done before

Tasks:

  • Proxy arrays, kill faux array
  • Proxy maps
  • Proxy objects
  • Introduce sets
  • Update / finetune observable api
  • Verify performance
  • Document migration path
  • Re-evaluate existing api, remove deprecated stuff, deprecate things to be removed
  • Update docs, tutorials, gotcha pages
  • Double check if all issues referred to in this issue are adressed
  • Clearly communicate environment dependencies of 4.0, suggest 3.0 branch for projects with strict browser requirements
  • Clean up the legacy exports (for default and *)
  • Decompose extras namespace for better tree shaking
  • Remove spy / or spy events related to object mutations [discuss]
@stevefan1999-personal
Copy link

long live the MobX

hopefully it could defeat Redux in one day

@andykog
Copy link
Member

andykog commented Jul 4, 2017

bq. Proxy arrays, kill faux array
@mweststrate, do you really think of dropping IE support? :-( My granny wont be able to visit my website :-(

@mweststrate
Copy link
Member Author

@andykog I think 3.0 should be maintained still, but want to prepare for the future as well. Promised for a long time now that certain issues will be fixed when proxies are available, and their support is getting more decent now :)

@marvinhagemeister
Copy link

marvinhagemeister commented Jul 4, 2017

@mweststrate Although we do have some clients who use IE11 explicitly, I do very much support going all in with Proxies for mobx. I'm really excited about them and think they will make the core a lot simpler.

@andykog
Copy link
Member

andykog commented Jul 4, 2017

Not everybody is so lucky that they can ignore IE users. You can't tell managers that we have to drop about 4% of customers because that will make mobx core simpler. This puts many user in a position where they have to decide wheather they gonna stick to an older mobx version or use something different for their apps. My opinion is that this is to early for switching to proxies.

@Keats
Copy link

Keats commented Jul 6, 2017

Compatibilty of:

I don't think I would be able to switch immediately to v4 but I would support using proxies and Map anyway, it's not like 3.0 is going to stop working.

@mattruby
Copy link
Member

mattruby commented Jul 6, 2017 via email

@andykog
Copy link
Member

andykog commented Jul 6, 2017

+1 for mobx-next, thought about that as well

@jamiewinder
Copy link
Member

jamiewinder commented Jul 6, 2017

I could be wrong, but I think

  1. Maps only supporting primitive keys and Sets not being present

can be implemented now without any loss of browser support (with a polyfilled ES6 Map)? If so, it might be worth this - and any other breaking additions which needn't affect browser support - being the basis of v4, and v5 (or mobx-next) forming the basis for the big move to using Proxy?

@stevefan1999-personal
Copy link

https://github.com/ascoders/dynamic-object

here's an object observer with proxy that features similar functionality like mobx, using almost purely Proxy. Indeed the project is written by a Chinese, hence I doubt your ability to read the documents and comments, but you guys can take it as a code reference.

@Keats
Copy link

Keats commented Jul 7, 2017

I would be against mobx-next that sounds like it is a completely new API etc. I don't really see the issue with using v4 for breaking changes, there are polyfills for Map and maybe for proxy (https://github.com/GoogleChrome/proxy-polyfill). v3 will still work and would likely be in maintenance mode

@urugator
Copy link
Collaborator

Some quick notes:

  • Change expr to accept options object (like computed) instead of scope
    (breaking)
  • Unify naming for "this" option, in computed/reaction it's called context, in autorun/action/when/autorunAsync/spy it's called scope
    (non-breaking)
  • Provide dispose(reaction object) as a second argument to reaction's sideEffect callback
    (non-breaking)

@mweststrate
Copy link
Member Author

@urugator tnx!

Also: Upgrade to latest TS

@Strate
Copy link
Contributor

Strate commented Jul 18, 2017

Can we avoid "dispose" somehow, for example storing listeners to WeakMap with observable as a key?

pseudocode:

const map = new WeakMap()
observe(obj, listener) {
  map.set(obj, listener)
}

@acronoah
Copy link

Managing reactions/autorun disposal is my one difficulty with Mobx. Especially when the autorun is on something other than a component (and thus without an unmount/dispose...)

@iamdanthedev
Copy link

Having struggled recently with async computed functionality, I woukd propose it out of the box.

@andrew-luminal
Copy link

@rasdaniil +1

Better, clearer support for async, both for computed and for action, would be high on my list. I don't know how feasible it is to improve, but runInAction feels very kludgy to have to use after every await in an async action.

@mweststrate
Copy link
Member Author

mweststrate commented Jul 27, 2017 via email

@tkrotoff
Copy link

tkrotoff commented Aug 3, 2017

@andykog

You can't tell managers that we have to drop about 4% of customers

MobX releases:

By the time MobX v4 is released, IE usage will be lower.
+ a few more months because you prefer a bit of dust on new things
+ the time it takes to switch to v4 (upgrade code, test, document, release)
=> IE will be even lower

Voilà

@aliakakis
Copy link

aliakakis commented Aug 4, 2017

@stevefan1999 Redux is OK but still, I do get quite intrigued when I listen as an argument that Redux is for large apps and MobX for small/medium. Not to mention that after a while the amount of files and complexity a Redux based project has to deal with is absurd.

@mweststrate 👍 for killing faux arrays

@OshotOkill
Copy link

Good time to looking for the future. I approve the plan of switching to proxies underneath. Besides, React 16 requires Map & Set in runtime environment and it's mandatory.

@jamiewinder
Copy link
Member

@OshotOkill - But Map and Set can be polyfilled. Proxy cannot.

@phiresky
Copy link
Contributor

phiresky commented Sep 1, 2017

@jamiewinder But wouldn't the existing partial polyfill for Proxy cause a rewritten version of mobx to behave just like the current version does (i.e. new properties are not tracked)?

Then mobx4 could just have a compatibility flag that disallows adding new properties to objects and browser compatibility would stay the same.

@jamiewinder
Copy link
Member

jamiewinder commented Sep 4, 2017

@phiresky I'm not really sure what the extent of the limitations of that polyfill are apart from the ones you highlighted. One that does start out is the faux array issue of MobX - observable arrays are not arrays. If MobX were to rely on a proxy polyfill, then we'd be in an ever more precarious position where an observable array may or may not 'be' an array depending on whether you're using the polyfill.

I think the switch to proxies should be absolute. i.e. if you need to support non-proxy browsers, then I'm afraid you'll have to stick to the previous version. However, I'm very likely one of those people who'll have to do this, and I'm missing some of the other features that are hoping to be added such as non-string keyed observable maps which can be added but will be a breaking change.

It'd be nice (though I wouldn't presume to ask for it) to see a MobX 4 that adds these features (and require polyfilled Map, Set), and a MobX 5 that forces you to use Proxy, but this might lead to confusion and having to maintain two active versions. So I'm not sure.

@mweststrate
Copy link
Member Author

mweststrate commented Sep 5, 2017 via email

@mweststrate
Copy link
Member Author

For implementation details see https://github.com/ascoders/dob, #776

@solkimicreb
Copy link

Just a quick note, because I see people mentioning only IE here. React Native is still not supporting Proxies, which is more important then IE in my opinion (other platforms are fine). Does anyone know when they plan to start supporting them?

@farwayer
Copy link
Contributor

farwayer commented Sep 5, 2017

@solkimicreb As I know Proxy supported in iOS starting from 10.0. I didn't tested yet but it should be available on iOS because React Native uses system JSCore. On Android Proxies is not supported yet.

@mweststrate
Copy link
Member Author

I like the second example the best as it is straight forward, but I think the trickyness is the need to double declare every field (or maybe only the non observable ones? that would make it a lot more bareable)

The third example is not really an improvements, as it keeps the same type juggling (ref.with(Buffer) doesn't return a Buffer) and has again the decorator working on the values, not on the properties. Basically the same as we already have (currently you would write observable.ref(Buffer))? Maybe what we have now isn't that bad at all, but if we can approve, now is probably the time :)

But unifying decorate / extendObservable like in the second example is an interesting direction. It would turn the earlier example into:

decorate({
   field: 3,
   field2: Buffer,
   get c1() { },
   get c2() { expr },
   method() {}
}, {
   // no need to decorate 'field', observable is the default
   field2: observable.ref, // needs decorating 
   // no need to decorate 'c1', getters are assumed to be computed
   c2: computed({ opts }), // computed decorator with option
   method: action.bound // method fields need decoration to turn them into actions (otherwise they are assumed to be views)
})

The nice thing about this is that no values appear in the second argument, which keeps it very consistent with decorators that also don't work on the actual values (decorating is done before field initialization), and it indeed shouldn't make any difference whether the first argument is an object, class, etc etc, semantics would always be the same. Biggest downside is see is that declaring a lot of actions is verbose, the other variations are probably rare enough to justify needing to be 'declared twice'

(Type-checking note to self: every key in decorators arg must be in target arg, except for observable(.X) when targeting classes, because with classes the field doesn't exist before the constructor has run)

@urugator
Copy link
Collaborator

urugator commented Feb 28, 2018

the decorator working on the values, not on the properties.

Decorators work on properties by definition. It simply accepts options, exactly the same way as computed ... computed.equals(fn) doesn't return fn ... does it mean it works on values instead of properties? nonsense...

Don't allow to pass anything, but decorator, to decorate. Only function which accept values (but not decorators) should be observable:

decorate(observable({
  a: 5,
  field: Buffer,  
}), {
  field: observable.ref,
})

You may preserve extendObservable, which works like observable (only values, no decorators), but "extends" existing object:

extendObservable(existing, {
  a: 5,
  field: observable.ref // NOPE! it stores decorator to field as value, use decorate instead
})

@mweststrate
Copy link
Member Author

the decorator working on the values, not on the properties.

to clarify: what I mean is that if all fields are declared in the target argument, the type can be inferred correctly from that. However, once values are declared in the second argument, the second argument is required to infer the correct type, and to even achieve that the type system must be tricked into the decorator not returning a descriptor, e.g.:

// a.x can be easily inferred to a number
const a = decorate({ x: 1 }, { x: observable.ref }) 

// this api would need to check the second argument for type inference, 
// *and* make `.with` return the type of it's value, instead of the decorator with options it actually returns
const b = decorate({}, { x: observable.ref.with(1) }) 

@urugator
Copy link
Collaborator

a.x can be easily inferred to a number

Is it useful for people who don't use typescript/transpiler(no type checking), therefore don't have an access to decorators, therefore are the ones using decorate?

Is it doable in way that I can trade verbosity for type checking? So if the value is in the first arg, I have more verbose code, but strict typing...?

@mweststrate
Copy link
Member Author

mweststrate commented Feb 28, 2018

Is it useful for people who don't use typescript/transpiler(no type checking), therefore don't have an access to decorators, therefore are the ones using decorate?

No, that set of people is disjunct. Many people use flow without decorators, or typescript implicitly without decorators (when using VS code in a plain javascript project it will still use the typescript compiler to infer type information)

Is it doable in way that I can trade verbosity for type checking? So if the value is in the first arg, I have more verbose code, but strict typing...?

Yes that would be possible. But I am wondering, is the verbosity that bad? The only case where one must double declare the fields, is with observable.X and computed.X.

For actions that is not strictly necessary, because extendObservable({}, { x: action(fn) }) yields type & semantically the same as decorate({ x() {} }, { x: action })). (action is more about the value itself than the property that owns it; action properties are not writable)


In other words, I think we could largely follow your proposal in unifying extendObservable and decorate, even without .with. extendObservable could roughly become:

extendObservable(target, values, decorators?) {
   // infer decorators from values
   for (key in values) {
      if (!(key in decorators)) {
         if (hasGetter(values, key))
            decorators[key] = computed
         if (!isFunction(values[key]))
            decorators[key] = observable
      }
   }
   // decorate!
   return decorate(Object.assign(target, values), decorators)
}

Advantages: type safe decorate & extendObservable. observable.ref etc could become purely decorators, not factories that produce a decorator application description when invoked on a value rather than a property

@urugator
Copy link
Collaborator

urugator commented Feb 28, 2018

I like it, some thoughts:

  • It won't work on prototypes (because values would be assigned to prototype as static prop, not lazily to instance in decorator ...)
  • Could we use Object.getOwnPropertyNames/Descriptors instead of for-in? So that following is possible:
extendObservable(this, this, { // getters are not enumerable in classes
  ref: observable.ref        
  method: action,
})
// EDIT: ahh, it wont' work ...these're not own, but on prototype, right?
  • observable should also accept decorators (since there are no modifiers...). I quess there is overloading problem ... so maybe move decorators to mobx.decorators ... or ... is @observable still supported or dropped in favor of .deep/.ref?
observable({
  b: Buffer,
}, {
  b: decorators.ref // or decorators.observableRef ?
})
``

@mweststrate
Copy link
Member Author

mweststrate commented Feb 28, 2018

It won't work on prototypes (because values would be assigned to prototype as static prop, not lazily to instance in decorator ...)

Decorate would work fine afaik? extendObservable not, but I think that is fine?

Could we use

Sure, this is just rough draft code :)

I quess there is overloading problem

I don't think so, when used as decorator, the second arg is always string, clearly distinguishable from
the decorator map

@urugator
Copy link
Collaborator

Decorate would work fine afaik? extendObservable not, but I think that is fine?

It's not like that extendObservable does't work, but decorate is simply
extendObservable(clazz.protoype, {}, decorators); do we need a special method for that?
Also I have been thinking that people may actually prefer different, perhaps more friendlier, syntax, like:

// similar to mobx-react's inject
decorate({ 
  a: observable,
  method: action,  
})(class Store {
  constructor() {
    this.a = 5;
  }
  method() {}
});

So maybe we should leave the actual decorate implementation up to users and only provide some hints in docs?
(Actually I think that "the sanest" solution is class/model factory similar to one in mobx-state-tree, but I would leave that to users as well...)

I don't think so

Then that's a good news :) Hopefully it's also compatible with new decorator proposal(s) ...

Can we shorten extendObservable to just extend or is it too ambiguous?

@mweststrate
Copy link
Member Author

@urugator see #1365 for the impact of the new api (especially the tests section)

@mweststrate
Copy link
Member Author

Hi all, if you want to experiment with the new Mobx 4 api, you can try it now!

Just install [email protected]

The changelog can be found here

Give it a try! Let me know how much the impact is of the api changes :).

You might note that this version is slower then mobx3, but that is just a side effect of the recent api refactoring, mobx4 will be faster then mobx 3 (alpha.1 is representative performance wise)

@pkieltyka
Copy link

@mweststrate very exciting! Btw, as a TypeScript user myself I have a quick question: will the "experimentalDecorators" offered in TS still work with mobx4 in the convenient form @observable field = 3 ?

@mweststrate
Copy link
Member Author

mweststrate commented Mar 1, 2018 via email

@urugator
Copy link
Collaborator

urugator commented Mar 1, 2018

About computeds:

  • remove compareStructural option, it's redundant (2 two ways to do the same thing, complicates computed impl, unnecessary dependency)
  • imho there is a diference between: deep and structural equality...
    Deep means that it traverses traversable values (object/array/set/map), but uses standard (identity) comparator for leafs (non-traversable-values).
    While structural equality should try to convert everything to it's value representation first and then traverse the value if it's traversable.
    Standard way to get the value representation in JS is to call valueOf. It returns identity if not implemented - meaning that the object itself (the reference) is the value:
// Buil-in value types
console.log(new String("hello").valueOf() === "hello"); // true
console.log(new Number(5).valueOf() === 5); // true (NaN requires a special treatment)
console.log(new Date(1519916831621).valueOf() === new Date(1519916831621).valueOf()) // true
console.log(new Date(1519916831621).valueOf() === 1519916831621); // true
// Arbitrary value types
const arbitraryValueLikeObject1 = Mobx.observable.box("a");
const arbitraryValueLikeObject2 = Mobx.observable.box("a");
console.log(arbitraryValueLikeObject1.valueOf() === arbitraryValueLikeObject2.valueOf()); // true

So what I propose:

  • distinct between deep and structural
  • adjust naming accordingly
  • adjust logic of structural comparator to rely on valueOf to (perhaps) simplify implementation and to support arbitrary value-like objects (box,Point,Vector,...)
  • adjust logic of deep comparator to always use identity comparator on non-traversables

@urugator
Copy link
Collaborator

urugator commented Mar 1, 2018

Not sure if it's just temporal, but I've noticed we're accessing process.env.NODE_ENV on multiple places. This has according to react's creators huge impact on performance when running on server, please change it to single call...

Unfortunately, it turns out that process.env is not a normal JavaScript object, and it is quite expensive to get a value out of it. So even when the value of NODE_ENV is set to production, just checking the environment variable so frequently adds a significant amount of time to server rendering.

@mweststrate
Copy link
Member Author

remove compareStructural option

agreed

imho there is a diference between: deep and structural equality...

I don't think in mobx we don't really need to make this distinction (this can always be done in a custom equals function), because the only case where the difference would actually be different, is where an non-plain object implements a valueOf method.

Did you note that 3.6.1 / 4.0.0 had a revision of the deep-equality mechanism because it contained a bug? It now follows the underscore implementation. This means that even non-primitive objects are traversed, and things like Point & Vector should work fine as well

@mweststrate
Copy link
Member Author

For the NODE_ENV: the point of it is that it is compiled away during minification (see the minified build in your node_modules). As soon as a variable is introduced to capture that value, the uglifier won't remove the code anymore. So the process.env will only be checked in a development (or in an wrongly set up product, but we can detect that)

@urugator
Copy link
Collaborator

urugator commented Mar 1, 2018

This means that even non-primitive objects are traversed, and things like Point & Vector should work fine as well

Unless their internal structure is different from what their valueOf returns...
If you don't want to go this route I would at least suggest to rename it to deep ... it's shorter, it uses deepEq function, works like _.deep ... so why structural?

So the process.env will only be checked in a development (or in an wrongly set up product, but we can detect that)

I am probably missing something ... when I require("mobx") on the server (there is never any compilation step) it always uses normal non-minifed version (mobx.js) right? And this version contains these env reads or not...?

@mweststrate
Copy link
Member Author

... to rename it to deep

agreed, will do

on the server

the main concern is a smaller build, not performance. When running the performance test suite on the server with and without the process.env.NODE_ENV substitution, there was no measurable difference in the speed, so if this was a problem, it seems solved by now (or mobx doesn't have enough checks yet ;-)).

@mweststrate
Copy link
Member Author

Mobx 4 beta is now available! npm install [email protected]. Migration guide: https://github.com/mobxjs/mobx/wiki/Migrating-from-mobx-3-to-mobx-4

@urugator
Copy link
Collaborator

urugator commented Mar 5, 2018

...so it's stil structural... :)

@mweststrate
Copy link
Member Author

mweststrate commented Mar 6, 2018

@urugator correct, I started renaming, but then it got weird because that would involving renaming computed.struct to computed.deep which makes it less clear what it does..

@urugator
Copy link
Collaborator

urugator commented Mar 6, 2018

I thought that computed.struct is also removed ...same as computed.equals...

@mweststrate
Copy link
Member Author

mweststrate commented Mar 6, 2018 via email

@urugator
Copy link
Collaborator

urugator commented Mar 6, 2018

I see, I thought it's @computed(options), you have a mistake in migration guide I believe, check the new API for @computed.equals(compareFn) :)

@mweststrate
Copy link
Member Author

mweststrate commented Mar 7, 2018

Released:

[email protected]
[email protected]
[email protected]

Which together fix some build & typescript issues :)

@mweststrate
Copy link
Member Author

released [email protected] (fixing #1376)

@mweststrate
Copy link
Member Author

Released 4.0.0!

@marvinhagemeister
Copy link

@mwestrate awesome! Congrats for the ride! Really happy with Mobx :)

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