Road to 4.0

[mweststrate]

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 #800

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

Breaking changes

• observable(object) will now make all properties (including future ones) observable. To limit / specify the set of observable properties one will have to use extendObservable
• Some utilities like isArrayLike will be deprecated
• Array.move will be deprecated
• Improve some typings #818, #817

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]

[mweststrate]

Thanks for the responses on the quick brainfart! I'll open a separate issue to discuss the above. But in short, the goal of the proposal is as follow:

1. Currently the default is deep observability, or the lazy approach
2. I notice that as a result, people tend to make to many things observable, and don't grasp the conceptual between observables references / properties, and values. This is a constant source of trouble / questions for beginners.
3. So I would like the API to encourage thinking about what actually should be observable. For example, @observable things = [] is an approach that always works, but hardly brings real understanding on how mobx works. However, when somebody uses @observable.ref things = [] or readonly things = observable.array() this nicely captures the reasoning behind the state shape design.
4. Repurposing practically the same api might be very confusing during migration. Although probably removing the @observable shorthand could be enough?

[spion]

I think that rewriting https://mobx.js.org/getting-started.html for a new API proposal highlights all the problems with new proposals if there are any, very nicely :grinning:

[urugator]

Move asyncAction to mobx package, because it's a must and I don't want to import utils. Maybe re-consider naming because there is nothing like "async action" (sort of contradiction in terms), only a series of sync actions interlaced by async operations. Maybe actions would be enought? Or sequence/series? Dunno, maybe I am just overthinking it.

[damaon]

Maybe asyncAction can be named effects?

[mweststrate]

@urugator: Imlemented decorate https://github.com/mobxjs/mobx/pull/1321/commits/bbc31c868407ee6f3b2371d82a15551d0c23cd28

[urugator]

@mweststrate Not sure about the "class only" limitation. I mean it decorates the object, does it really need to have a constructor?

// OLOO
const Box = {
  sizes: [2],
}
decorate(Box, {
  sizes: observable
});
const box = Object.create(Box);

// or are there any issues with:
const box = decorate({
  sizes: [2],
}, {
  sizes: observable,
})

I think it would be handy to return decorated object (or class if we insist)

[mweststrate]

@urugator In principle that is possible. But at this point

const box = {
  @observable sizes: [2]
}

is also officially not supported (I think it works though). The reason that pattern isn't supported (yet) is that the decorators proposal currently in stage 3 is significantly different from the babel / ts transformations we now rely on. (It is much better) but for now I want to avoid extending the current decorator proposals.

That being said, I think the way of creating objects your are proposing is nicer then extendObservable with all the modifiers :) I'll add some unit tests and check how hard it is..

[mweststrate]

@urugator ok, that was easy: https://github.com/mobxjs/mobx/pull/1321/commits/2c88f3344573f7d0ce7657c69ba8491d036420d6

[urugator]

@urugator Hm, I thought that simply param type should change, so that user has to call decorate(Box.prototype, decorators). It's impossible to decorate functions like this. It's not uncommon to use functions as objects in JS (static props, functors). Having seperate decorateClass would be cleaner design wise I think (instead of type inference), but I don't see a problem in requiring object(prototype) from user.

export function decorate<T>(target: any, decorators: any) {
    invariant(isPlainObject(decorators), "Decorators should be a key value map")    
    for (let prop in decorators) {
        const decorator = decorators[prop]
        invariant(
            typeof decorator === "function",
            `Decorate: expected a decorator function for '${prop}'`
        )
        const descriptor = Object.getOwnPropertyDescriptor(target, prop)
        const newDescriptor = decorator(target, prop, descriptor)
        Object.defineProperty(target, prop, newDescriptor)
    }
    return target;
}

[mweststrate]

decorate feels like we don't need extendObservable alltogether. Or am I going to wild now :-P?

[urugator]

The main advantage of extendObservable (aside being slightly less verbose) is that everything (key, value, observability) is defined at one place, which makes it conceptually closer to decorators and therefore easier to maintain (you don't have to manage props at two places). That's why I was wondering if we could add "initialValue" option to decorators:

const o = { 
  b,  
}
decorate(o, {
  a: observable({ value: "initial" }),
  b: observable({ value: "initial" }), // throw if b is already defined and "value" opt is passed? 
  c: observable(), // throw if c is not defined and "value" opt is not passed?
  // or maybe we don't need options object? so keep current syntax, but return "enhanced" decorator instead of "modifier"
  c: observable("initial"),
})

EDIT: Simply put, the idea is to create descriptor from passed value, when there is no descriptor and value is provided.

[mweststrate]

I don't think we can plumb even more overloads of observable into the api and still give useful results or error messages :)

Op vr 23 feb. 2018 om 14:12 schreef urugator notifications@github.com:

The main advantage of extendObservable (aside being slightly less verbose) is that everything (key, value, observability) is defined at one place, which makes it conceptually closer to decorators and therefore easier to maintain (you don't have to manage props at two places). That's why I was wondering if we could add "initialValue" option to decorators:

const o = { b, }decorate(o, { a: observable({ value: "initial" }), b: observable({ value: "initial" }), // throw if b is already defined and "value" opt is passed? // or maybe we don't need options object? so keep current syntax, but return "enhanced" decorator instead of "modifier" c: observable("initial"), })

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx/issues/1076#issuecomment-368005307, or mute the thread https://github.com/notifications/unsubscribe-auth/ABvGhKdyO9ixeY1WTDyg5ZTN6V2xBMjBks5tXrkngaJpZM4ONAsS .

[urugator]

Yea, I am just trying to find out whether it could be solved with "higher-order-decorator"

decorate(o, {  
  a: withValue("initial", observable),
  // the same
  b: (target, key) => observable(target, key, { value: "initial" })
})

function withValue(value, decorator) {  
  return function withValueDecorator(target, key) {        
     return decorator(target, key, { value }); 
  }
}

EDIT: If the above works, then removing extendObservable doesn't seem to be a problem (to me)... EDIT: changed so that the original descriptor is always ignored

[urugator]

Or would it be weird to expose method on decorators? (computed does it already)

decorate(o, {  
  a: observable.with("initial"), 
})

[mweststrate]

@urugator I think the fact that decorate does not support initial values now is a feature (unless we kill extendObservable). The neat thing is that it kind of forces you to still do the field declarations in your classes (I think that will be the most important use case for decorators). That might sound as disadvantage at first, but the benefit is that you get proper type checking in Flow and Typescript. (Which is a problem with extendObservable in constructor which is currently the typical way to avoid decorators)

[mweststrate]

Released a first alpha of mobx4: mobx@4.0.0-alpha.1. Messy release notes can be found here: https://github.com/mobxjs/mobx/blob/mobx4/CHANGELOG.md

Note that the release is not complete yet (still some pending api updates, and will add some backward compatibility api's). Progress can be followed here: https://github.com/mobxjs/mobx/pull/1321

[urugator]

@mweststrate If I would use typescript/transpiler I wouldn't need decorate in the first place don't you think? (in other words if you have to use decorate you most likely don't have an access to these "benefits", not even to field initializers!) Really let me put decorate wherever I want, without making assumptions:

class Store {
  constructor() {
    decorate(this, {
      a: observable.with("a"); 
    })
  }
}

decorate(Store.prototype, {
  b: observable.with("b");
})

unless we kill extendObservable

I would probably kill right side modifiers altogether, even for observable ... if you want to modify behavior use decorate ... (and thats when initial value helper comes in handy)

[mweststrate]

Here is a nice sandbox to try the alpha: https://codesandbox.io/s/1oj3zzk0j7

[mweststrate]

I would probably kill right side modifiers altogether, even for observable ... if you want to modify behavior use decorate ... (and thats when initial value helper comes in handy)

That was what I was thinking about as well, but after a quick investigation many seem to be using it

The first usage of decorate in your example is something I would really like to discourage, it's goal is to be used on types not on instances, to keep it semantically close to how decorators work, and will be working

[phiresky]

Question about the alpha: What is the difference between keys/values and Object.keys and Object.values ?

[mweststrate]

keys and values can be tracked and will react to future property additions / removals on observable objects. See for example ( https://t.co/VdVwkK159S).

MobX5 immediately will obsolete this again (by leveraging Proxies). The reason I added it is mainly if people need to migrate back from Mobx5 (cause, Internet Explorer), and this way MobX4 can offer feature parity.

Op di 27 feb. 2018 om 21:56 schreef phiresky notifications@github.com:

Question about the alpha: What is the difference between keys/values and Object.keys and Object.values ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx/issues/1076#issuecomment-369022859, or mute the thread https://github.com/notifications/unsubscribe-auth/ABvGhFVEIFU7e0rQEi0W7DSBI_yXeYEvks5tZGvhgaJpZM4ONAsS .

[urugator]

@mweststrate

quick investigation many seem to be using it

And what else should they be using? I obviously use it too (I don't use decorators btw), the discussion is about whether it could be replaced by decorate, which does exactly the same thing - makes existing things observable ... so that we don't have 2 ways to do the same thing.

discourage, it's goal is to be used on types not on instances

If this would be possible:

const o = { 
  @observable a: "";
}

would you still discourage:

decorate(o, {
  a: observable,
})

The decorators are not limited to classes by design. The @observable syntax is (currently). It doesn't make sense to artifically introduce the same limitation for decorate function (or do I miss some technical limitation perhaps?). Decorators work with objects (not with types/instances) - whether the object is used as a prototype or not is irrelevant from the perspective of decorator.

But primarily, we don't need a function for applying decorators. We need a function for making objects observable. The idea behind decorate is that we simply reuse decorators to do so ... Or let me put it like this: Why would I want to use decorate instead of extendObservable?

Btw I realized there is no descriptor for property decorator, so that higher-order-decorator idea is wrong right? (or maybe there is for ESNext?)

[mweststrate]

@urugator let me take a step back and explain what me annoys me about the current api; it is that (extend)Observable have become as overloaded as jQuery and to all kind of juggling around with typings. Take the following examples:

Decorators

class A {
   @observable field = 3
   @observable.ref field2 = Buffer
   @computed get c1() { }
   @computed({opts}) get c2 {}
   @action.bound method() {}
}

These are concise, quite intuitive and straightforward transformations. There is nothing going on with the values and types (ok, not true for observable, but with proxies the types will be come consistent as well)

Decorate

class A {
   field = 3
   field2 = Buffer
   get c1() { }
   get c2 {}
   method() {}
}
decorate(A, {
   field: observable,
   field2: observable.ref,
   c1: computed
   c2: computed({ opts })
   method: action.bound 
})

Now this is still straightforward, has literally the same concepts, and is even pretty close to what your compiler would emit when supporting decorators. It attaches behavior to the fields, but does change their signatures.

Obviously the problem here is that the double map doesn't read nice. It's not concise anymore and a little error prone.

extendObservable / observable

const a = observable.object({
   // this is fine
   field: 3,

   // this is weird, observable.ref "packs" the Buffer into a special object, so that `object` recognizes it,
   // can unwrap the value, and also attaches the right behavior to 'field2'
   // the type checker has to be tricked into thinking `ref<T>(t: T): T`, while actual signature something like
   // `ref<T>(t: T): { value: T, "$$refWrapper": true }
   field2: observable.ref(Buffer),

   // this is fine as well
   get c1() { },

   // this again is weird, `computed` just creates a normal ComputedValue object
   // which, actually has a completely separate signature
   // but `object` detects than, and folds them into the target object
   // be aware of arrow functions though; they will have the wrong `this`
   c2: computed(function () { expr }), { opts })

   // seems ok-ish, because it doesn't mess with the types
   // but again the => trap!
   // but, action.bound again has to specially mark the create function, 
   // to signal that `object` it needs to unwrap, bind, and wrap the function again
   method: action.bound(function () { })
}

And maybe it is all not that bad as I think it is, but to me it feels that there are to many inference rules in extendObservable / object.

The typing problem is by the way probably fixable in the near future as TS is about to introduce mapped types.

So indeed, I agree that @observable should work on objects as well, my point was more or less: Ideally I would see all those decorators only operating on the definition of the object, not it's values. (indeed, not on the right hand side of assignments).

So ideally, but not sure it is realistic, I would get rid of extendObservable with it's many rules, but replace it with something that is not much more verbose

[mweststrate]

Could something like this work?

extendObservable({}, {
   field: 3
   field2: [observable.ref, Buffer],
   get c1() { },
   c2: [computed({ opts }), function() { }]
   method: [action.bound, function() { } ]
})

[jamiewinder]

Is the change for non-string keyed ES6-like observable maps (https://github.com/mobxjs/mobx/pull/800) making it into MobX 4? I think the only thing putting it off was that it was a breaking change (since adding a key of numeric 1 vs string '1' is now considered different). It seems it'd be a good segway to the proxies mode where presumably proxied ES6 maps will have this behaviour anyway.

[mweststrate]

Good one! Added it to the list

Op wo 28 feb. 2018 om 11:50 schreef Jamie Winder notifications@github.com:

Is the change for non-string keyed ES6-like observable maps (#800 https://github.com/mobxjs/mobx/pull/800) making it into MobX 4? I think the only thing putting it off was that it was a breaking change (since adding a key of numeric 1 vs string '1' is now considered different). It seems it'd be a good segway to the proxies mode where presumably proxied ES6 maps will have this behaviour anyway.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx/issues/1076#issuecomment-369202017, or mute the thread https://github.com/notifications/unsubscribe-auth/ABvGhMhaV0ir6AjikU9YyPO5_gxoAX91ks5tZS9pgaJpZM4ONAsS .

[urugator]

Could something like this work?

I really don't get it. Why extendObservable needs a different way of "decorating" (array, current modifiers, whatever)? Why extendObservable can't use normal decorators as well? Then it just makes sense to rename it to decorate, because all it does is applying decorators ... thats the whole idea all along ... To provide initial value (or any settings in general) you just need a parametrized decorator (decorator factory) ...

// weird juggling
extendObservable({}, {
  a: 5,
  b: observable.ref(Buffer),
})

// key: decorator
// initial value must be defined on object   
extendObservable({ a: 5, b: Buffer }, {
  a: observable,
  b: observable.ref, 
})

// key: decorator (created by factory)
// initial value passed to decorator factory
extendObservable({}, {
  a: observable.with(5) // returns good old decorator! 
  b: observable.ref.with(Buffer) // returns good old decorator! 
})

// so if it just applies decorators to given object...
decorate(object, decorators); // object (no class)! 

If you keep extendObservable and just introduce decorate then nothing has been solved ... the weird juggling still exist and there is just a new method (decorate), which does the same thing as extendObservable, but is limited to classes, more verbose, less expressive, more error prone and harder to maintain ... so again why should I use it instead?

[mweststrate]

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]

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]

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]

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]

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]

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]

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]

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]

@urugator see https://github.com/mobxjs/mobx/pull/1365 for the impact of the new api (especially the tests section)

[mweststrate]

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

Just install mobx@4.0.0-alpha.2

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]

@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]

Yes!

Op do 1 mrt. 2018 15:39 schreef Peter Kieltyka notifications@github.com:

@mweststrate https://github.com/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 ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx/issues/1076#issuecomment-369612073, or mute the thread https://github.com/notifications/unsubscribe-auth/ABvGhAr7wxtUUosn7Q1vhCp_Kvyb__CQks5taAgsgaJpZM4ONAsS .

[urugator]

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]

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]

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]

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]

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]

... 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]

Mobx 4 beta is now available! npm install mobx@4.0.0-beta.1. Migration guide: https://github.com/mobxjs/mobx/wiki/Migrating-from-mobx-3-to-mobx-4

[urugator]

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

[mweststrate]

@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]

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

[mweststrate]

No, @computed.struct is the same as @computed.options({ equals: comparer.structural}). @computed.equals has been removed in favor of .options. But struct is such a common pattern that I think it deserves a short hand :-). The goal of the API is not to just be as small as possible, but to make the 80% use cases as easy as possible, and make the remaining 20% rest possible with an api that is as small as possible.

Op di 6 mrt. 2018 om 11:18 schreef urugator notifications@github.com:

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

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx/issues/1076#issuecomment-370733322, or mute the thread https://github.com/notifications/unsubscribe-auth/ABvGhGacoqvYZI-HR6acDa6LeNU97py_ks5tbmJkgaJpZM4ONAsS .