search

mobxjs / mobx

Simple, scalable state management.
http://mobx.js.org
MIT License
27.47k stars 1.77k forks source link

🚀 Proposal: MobX 6: 🧨drop decorators,😱unify ES5 and proxy implementations, 💪smaller bundle #2325

Closed mweststrate closed 3 years ago

mweststrate commented 4 years ago

MobX 6

Hi folks, I've tinkered a lot about MobX 6 lately, so I want to layout the vision I have currently

Goals

🧨 1. Become compatible with modern ES standards

Let's start with the elephant in the room. I think we have to drop the support for decorators. Some have been advocating this for years, others totally love decorators. Personally I hate to let decorators go. I think their DX and conciseness is still unparalleled. Personally, I am still actively engaged with TC-39 to still make decorators happen, but we are kinda back to square one, and new proposal will deviate (again) from the implementation we already have.

Dropping decorators has a few advantages, in order of importance (imho)

  1. Become compatible with standard modern JavaScript Since fields have not been standardized with [[define]] over [[set]] semantics, all our decorator implementations (and the decorate utility) are immediately incompatible with code that is compiled according the standard. (Something TypeScript doesn't do, yet, by default, as for TS this is a breaking change as well, unrelated to decorators). See #2288 for more background
  2. MobX will work out of the box in most setups MobX doesn't work with common out-of-the-box setup in many tools. It doesn't work by default in create-react-app which is painful. Most online sandboxes do support decorators (MobX often being one of the few reasons), but it breaks occasionally. Eslint requires special setup. Etc. etc. Dropping decorators will significantly lower the entry barrier. A lower entry barrier means more adoption. More adoption means more community engagement and support, so I believe in the end everyone will win.
  3. Less way to do things currently it is possible to use MobX without decorators, but it is not the emphasized approached, and many aren't even aware of that possibility. Reducing the amount of different ways in which the same thing can be achieved simplifies documentation and removes cognitive burden.
  4. Reduce bundle size A significant amount of MobX is decorator chores; that is because we ship with basically three implementations of decorators (TypeScript, Babel, decorate). I expect to drop a few KB by simply removing them.
  5. Forward compatibility with decorators I expect it will (surprisingly) be easier to be compatible with decorators once they are officially standardized if there is no legacy implementations that need to be compatible as well. And if we can codemod it once, we can do that another time :)

The good news is: Migrating a code base away from decorators is easy; the current test suite of MobX itself has been converted for 99% by a codemod, without changing any semantics (TODO: well, that has to be proven once the new API is finalized, but that is where I expect to end up). The codemod itself is pretty robust already!

P.s. a quick Twitter poll shows that 2/3 would love to see a decorator free MobX (400+ votes)

😱 2. Support proxy and non-proxy in the same version

I'd love to have MobX 6 ship with both Proxy based and ES5 (for backward compatibility) implementations. I'm not entirely sure why we didn't combine that anymore in the past, but I think it should be possible to support both cases in the same codebase. In Immer we've done that as well, and I'm very happy with that setup. By forcing to opt-in on backward compatibility, we make sure that we don't increase the bundle size for those that don't need it.

P.S. I still might find out why the above didn't work in the past in the near future :-P. But I'm positive, as our combined repo setup makes this easier than it was in the past, and I think it enables some cool features as well, such as detection of edge cases.

For example we can warn in dev mode that people try to dynamically add properties to an object, and tell them that such patterns won't work in ES5 if they have opted-in into ES5 support.

💪 3. Smaller bundle

By dropping decorators, and making sure that tree-shaking can optimize the MobX bundle, and mangling our source aggressively, I think we can achieve a big gain in bundle size. With Immer we were able to halve the bundle size, and I hope to achieve the same here.

To further decrease the build, I'd personally love to drop some features like spy, observe, intercept, etc. And probably a lot of our low-level hooks can be set up better as well, as proposed by @urugator.

But I think that is a bridge too far as many already rely on these features (including Mobx-state-tree). Anyway I think it is good to avoid any further API changes beyond what is being changed in this proposal already. Which is more than enough for one major :). Beyond that, if goal 2) is achieved, it will be much easier to crank out new majors in the future :). That being said, If @urugator's proposal does fit nicely in the APIs proposed below, it might be a good idea to incorporate it.

4. 🛂Enable strict mode by default

The 'observed' one, that is.

🍿API changes

UPDATE 22-5-20: this issue so far reflected the old proposal where all fields are wrapped in instance values, that one hasn't become the solution for reasons explained in the comments below

This is a rough overview of the new api, details can be found in the branch.

To replace decorators, one will now need to 'decorate' in the constructor. Decorators can still be used, but they need to be opted into, and the documentation will default to the non-decorator version. Even when decorators are used, a constructor call to 

class Doubler {
  value = 1 

  get double () {
    return this.field * 2
  }

  increment() {
    this.value++
  }

  constructor() {
    makeObservable(this, {
      value: observable,
      double: computed,
      increment: action
    })
  }
}

Process

Timeline

Whatever. Isolation makes it easier to set time apart. But from time to time also makes it less interesting to work on these things as more relevant things are happening in the world

CC: @fredyc @urugator @spion @Bnaya @xaviergonz

spion commented 4 years ago

You probably already know this :grinning: but here is my strong vote against removing decorators. I frankly don't care what TC39 think, decorators are here to stay. I'm not removing them from any codebase and I will strongly advocate against any removal from any library as well.

IMO its time to stand our ground.

Bnaya commented 4 years ago

Quick thought: What about adding decorators with external package to mobx core? "mobx-decorators" Do we expose the needed low-level primitives?

urugator commented 4 years ago

observable(1) cannot be <T>(value: T, options?) => T (as it must return some box), correct? How does this work with class field value: int = observable(1)?

I worry that this data/metadata duality will backfire eventually. If I remember correctly we had something like this in Mobx2.

Had an idea like: EDIT: nevermind bad idea...

xaviergonz commented 4 years ago

🧨 1. Become compatible with modern ES standards

Personally I also hate what TC39 is doing to decorators, and maybe it is one of those things that should be left to transpilers. Maybe there could be a mobx-decorators v7 package for those that want to keep using with them? (and also could make adoption easier).

😱 2. Support proxy and non-proxy in the same version

Sounds awesome.

💪 3. Smaller bundle

To further decrease the build, I'd personally love to drop some features like spy, observe, intercept, etc. And probably a lot of our low-level hooks can be set up better as well, as proposed by @urugator.

As long as there are alternatives it should be ok (mobx-keystone for example relies on both observer, intercept and then some more). But the more changes there are, the more likely current mobx libs will become incompatible and stop adoption.

🍿API changes

autoInitializeObservables will reflect on the instance, and automatically apply observable, computed and action in case your class is simple

When is a class considered simple? When there are no fields with mobx "decorators"? If so, it might be confusing to have a "simple" class, then add an action and see how the whole class becomes something totally different.

observable and extendObservable does currently convert methods to observable.ref's instead of actions by default.

I think 99% of the time you'd want functions to become actions (and actually I didn't even know this observable.ref was the case), so as long as this is explained on some release notes it should be ok. In the worst case there could be a global flag like "versionCompatibility": 5 or similar that actually makes it work as usual for those migrating from an older version and that prints in the console a warning when a function is passed to observer so you can eventually fix it and remove the flag.

kubk commented 4 years ago

To further decrease the build, I'd personally love to drop some features like spy, observe, intercept, etc.

mobx-logger depends on spy as well as mobx-remotedev (Redux devtools for Mobx). Is there another way to listen to observable mutations in Mobx?

danielkcz commented 4 years ago

You probably already know this 😀 but here is my strong vote against removing decorators. I frankly don't care what TC39 think, decorators are here to stay. I'm not removing them from any codebase and I will strongly advocate against any removal from any library as well.

IMO its time to stand our ground.

@spion Frankly, that is just a rant. Michel wrote the pros of removing decorators. If you want to "stand ground" and "strongly advocate", then please make constructive counter-arguments instead of just "I like them".

Personally, I am on the other side of this war and I never liked decorators. This fresh new look definitely makes sense to me. However, if there is some possibility to keep decorators support in the external package as it was suggested, then it's probably something we should do. If people feel the necessity to wear a foot gun, it's their choice. Besides, it would be suddenly more apparent where that decorator magic is coming from and that it's something optional.

webernir commented 4 years ago

April fools @mweststrate ?

mweststrate commented 4 years ago

No, being serious here 🙈. Missed opportunity though!

Op do 2 apr. 2020 06:50 schreef Nir Weber notifications@github.com:

April fools @mweststrate https://github.com/mweststrate ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx/issues/2325#issuecomment-607635350, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAN4NBGYSQOTJNWFY5ZPUYLRKQRTHANCNFSM4LZQ2B5Q .

mweststrate commented 4 years ago

I worry that this data/metadata duality will backfire eventually. If I remember correctly we had something like this in Mobx2.

Yeah, I think that is the part I like the least as well. Maybe we should introduce a separate 'marker' to for observable properties, e.g. field = tracked(value), and use observable only for instantiating collections (mostly relevant when not using classes). But not sure whether we should have an alternative for computed as well, and what would be a good name.

benjamingr commented 4 years ago

I like decorators but I totally understand this direction. There are a few reasons for this, let me try and make Gorgi's case @FredyC :

Also, removing something so widely used because the spec committee can't proceed always leaves a bad taste in my mouth. Even as I acknowledge they are all acting in good faith and that decorators are a hard problem for engines because of the reasons outlined in the proposal (I've been following the trapping decorators discussions).

benjamingr commented 4 years ago

Some API bikeshedding:

Decorators

class Doubler {
  @observable value = 1

  @computed get double() {
    return this.field * 2
  }

  @action increment() {
    this.value++
  }
}

Michel's original:

class Doubler {
  value = 1

  get double() {
    return this.field * 2
  }

  increment() {
    this.value++
  }

  constructor() {
    autoInitializeObservables(this)
  }
}

Subclass:

class Doubler extends ObservableObject {
  value = 1

  get double() {
    return this.field * 2
  }

  increment() {
    this.value++
  }
}

Can be interesting, but is not very good in terms of coupling. Or with a class wrapper:

const Doubler = wrapObservable(class Doubler {
  value = 1

  get double() {
    return this.field * 2
  }

  increment() {
    this.value++
  }
});
danielkcz commented 4 years ago
  • Decorators have been the "mostly standard MobX way" for a while and removing them is 5 years of breakage.

Don't forget there will be a codemod to make most of the hard work for you, so this isn't really a valid argument. Besides, nobody is forced to upgrade to the next major. It probably depends if we will be able to maintain v4 & v5 or abandon those. And if we separate package will exist for decorators then it might be fairly non-braking change.

Btw, @mweststrate Just realized, why MobX 7? Do we need to skip V6 for some reason? 🤓

mweststrate commented 4 years ago

Doh, I can't count.

Subclassing doesn't work, as the fields are not visible to the subclass when it runs its constructor. Wrapping is probably a bit scary for many people, but more importantly it is troublesome in TypeScript, as generic arguments aren't nicely preserved that way, and with circular module dependencies, as const expressions are not hoisted the same way as classes IIRC (that is an recurring issue in MST).

On Thu, Apr 2, 2020 at 10:56 AM Daniel K. notifications@github.com wrote:

  • Decorators have been the "mostly standard MobX way" for a while and removing them is 5 years of breakage.

Don't forget there will be a codemod to make most of the hard work for you, so this isn't really a valid argument. Besides, nobody is forced to upgrade to the next major. It probably depends if we will be able to maintain v4 & v5 or abandon those. And if we separate package will exist for decorators then it might be fairly non-braking change.

Btw, @mweststrate https://github.com/mweststrate Just realized, why MobX 7? Do we need to skip V6 for some reason? 🤓

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx/issues/2325#issuecomment-607744180, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAN4NBE6PWAMDR7QF4WVJFDRKROL7ANCNFSM4LZQ2B5Q .

benjamingr commented 4 years ago

@FredyC hey, I would prefer it if we avoided terms like "isn't really a valid argument" when talking about each other's points.

I think having a common and standard way to do something in a library (decorators) is definitely a consideration and API breakage is a big concern - even with a codemod. I think removing decorators is unfortunately the way forward - but breaking so much code for so many users definitely pains me.

@mweststrate subclassing is also not very ergonomic and mixes concerns here IMO.

I'm not sure I understand the wrapping issue in TypeScript but I know there are challenges involving it. Wrapping doesn't actually have to change the type similarly to initializeObservables:

class Doubler {
   ...
}
initializeObservables(Doubler); // vs in the constructor

Or even decorate the type in a non-mutating way:

const ReactiveDoubler = initializeObservables(Doubler); // vs in the constructor

Wouldn't it make more sense to initializeObservables on the type and not on the instance? Is there any case I'd want to conditionally make an instance reactive but not have several types?

mweststrate commented 4 years ago

@benjamingr yeah that is exactly what decorate does so far. The problem is that initializeObservables won't see the fields if invoked on the type, field x = y is semantically equivalent to calling defineProperty(this, 'x', { value: y }) in the constructor, so the field does never exist on the type.

So even if you don't know the fields, but you do specify them on the type, you can't decorate the type to trap the assignment, because the new property will simply hide it. I think it is still a weird decisions to standardize [[define]] over [[set]] semantics, which has rarely any merits, and deviates totally from what TS and babel were doing. But that is how it is.....

spion commented 4 years ago

@FredyC It is not a rant, it's a constructive comment. Decorators are widely used throughout the community, with large projects such as Angular, NestJS and MobX taking advantage of them. Thousands of projects depend on them. For TC39 to block their standardization process strikes me as extremely irresponsible, and the arguments for doing so are severely under-elaborated (a vague 3-pager does not an elaboration make - try harder TC39).

The advantages that @mweststrate mentioned are largely the fault of this lackluster standardization which means the argument is cyclic: it ultimately comes down to "we're not supporting decorators because decorators are not well supported". Language features that aren't yet standardized are never well supported, the argument can be used to justify not adopting any new language feature. So if TC39 "paves cowpaths" and everyone adopted this way of thinking, the language would stop evolving.

(Clearly, this is not a new feature so there is some merit to the "not likely to be supported" argument implying its a good idea to give up on them. I just wanted to bring to the table that the other approach - standing our ground - might be good too)

For those of us who do care about decorators, what are our options? Our only hope is to stand our ground, keep using them and keep advocating their standardization. Even if TC39 doesn't standardize them, development within TypeScript might continue, addressing the remaining gaps WRT reflection and types.

If you don't care about decorators, please stay out of it. They have always been optional in MobX and will continue to be optional - no one is forcing you to use them. If you care about a smaller bundle, they can be offloaded to a side module (but I maintain they should still have a first-class place in the documentation)

benjamingr commented 4 years ago

@mweststrate is there anything stopping us from trapping construct and intercepting those fields then or setting a proxy?

Such an initializeObservables wouldn't do anything until an object is constructed and then return a proxy (or decorate) when the constructor is called.

That is:

That way the fact the field is not a part of the type doesn't really matter - since while it looks like we are decorating the prototype/class we are actually decorating the instance and only "decorating" the construct on the type.

benjamingr commented 4 years ago

@spion

For TC39 to block their standardization process strikes me as extremely irresponsible, and the arguments for doing so are severely under-elaborated (a vague 3-pager does not an elaboration make - try harder TC39).

TC39 has legitimate considerations and concerns regarding decorators. TC39 does not owe us decorators and people have been collaborating in good faith.

Like most TC39 stuff - this is blocked on people actually doing the work and heavy-lifting to help address all the concerns (usability, implementation for engines, spec etc).

If you would like to help push decorators along I recommend you do the same thing as Michel and get involved. If you are not sure how to get involved I can happily connect you to some TC39 members that can help point you towards ways to contribute.

This (like most proposals and things) is harder than it sounds to do right, the default in TC39 for things that are half-baked is to not progress with them and force of will and determination is usually required.

spion commented 4 years ago

@benjamingr TC39 owes a lot more than a 4-page hand-wavy document. For a feature already used by thousands of projects including the most popular NodeJS framework and one of the most popular frontend frameworks. I would expect to see a proper design doc discussing the alternatives, pros & cons and elaborating on engine constraints in deep detail.

benjamingr commented 4 years ago

@spion I think we might have an expectation problem with how much TC39 owes anyone and us in particular. I also have a different expectation regarding how much detail parties have to provide to third parties like us. This is also true for projects (like Node.js) where the project has TC39 representation.

In any case these people aren't very far and they are pretty accessible, if you want to understand these concerns and get involved feel free to hit me up on FB and I'll connect you to members happily.

I would prefer to keep future discussion in this issue focused on the MobX 7 proposal and API bikeshedding :]

spion commented 4 years ago

I understand the concerns, but I see at least a few avenues that are left unexplored. I will likely try to get involved. (One of the unexplored options is "banned" syntax that will never be standardized, to guarantee that transpilers can do their compile-time decorator work, then TypeScript can proceed with confidence)

Regardless I think its extremely important for userland libraries and users to continue to signal that we care about decorators, precisely because TC39 has limited resources and has to prioritize based on some criteria. One of those criteria is likely interest.

danielkcz commented 4 years ago

Guys, let's not dive into TC39 problems here, this is about MobX. If you want to argue, take it at least to them, they won't see it here.

urugator commented 4 years ago

maybe we should introduce a separate 'marker' to for observable properties

That was sort of the idea :D. I used field = as(options, value), the type is infered from value/options combination, like { type: computed } or { computed: true }. But it still poses the problem with return type or not? For fake array, T=>T isn't possible as well...? It still seems to require a lot more "tinkering" than the seperate options map, which on the other hand looks quite clean to me. The only disadvanatage being it's not colocated. But my reasoning is that with default auto behavior one should need this extra map a lot less often and no one complained about decorate so far(?).

I also wanted to ask how that es5/next configuration works, does it require bundler/tree shaking?

mweststrate commented 4 years ago

For fake array/map, T=>T isn't possible as well...?

Strictly speaking not, but that has always been that way already, @observable field = [] doesn't expose the MobX utilities either, but generally that seems to be fine for everybody.

Edit: actually the proposal will make it better, gives some flexibility, because it could produce the signature T[] => IObservableArray<T> for example so that the utility methods do become visible. Not sure if that is a good idea, as that would make field assignments harder

It still seems to require a lot more "tinkering" than the separate options map, which on the other hand looks quite clean to me.

Yes, I agree, I'm still 60/40 on this approach. Using a decorate map is probably more efficient, and is definitely easier to migrate from and to decorators. The biggest drawback I really see, is that it is more prone to making mistakes. People already make a lot of mistakes with observer. And using a decorate map is very prone to both forgetting and refactoring errors, especially when not using TS. I'll update the proposal with this alternative though, for completeness.

mweststrate commented 4 years ago

I also wanted to ask how that es5/next configuration works, does it require bundler/tree shaking?

Yes, the idea is to put the entire ES5 implementation in a closure (or make sure that it is only referred by that closure), so that if the opt-in method is never called, it will be tree-shaken away. That is how it now works in immer

matijagrcic commented 4 years ago

Alternative API is great, the explicit action to
initializeObservables trumps the DX. And VSCode refactoring can probably make this an action based on code in the class.

urugator commented 4 years ago

The biggest drawback I really see, is that it is more prone to making mistakes.

Therefore the suggested auto behavior and strict checking by default. If there is a chance you forgot or misused anything it should immediately throw. Ideally you should just check console, if there isn't any error, everything should work as intended, if there are some, follow the instructions.

that would make field assignments harder

That was my concern (that it returns ObservableArray or BoxedPrimitive instead of just array/primitive), not the lack of utilities... just dunno how this is handled

szagi3891 commented 4 years ago 
class Doubler {
  value = observable(1)

  double = computed(function () {
    return this.field * 2
  })

  increment = action(function () {
    this.value++
  })

  constructor() {
    initializeObservables(this)
  }
}

Maybe it is worth reducing the memory consumption?

With each new instance of such a class, two new clouseras will be created. The problem is made when we have a lot of models and they have a lot of methods. Memory consumption increases exponentially.

A zero cost abstraction approach would be good.

mweststrate commented 4 years ago

@szagi3891 they are partially avoidable (see the notes), but that is definitely a good argument for the alternative api, where that is easier.

mweststrate commented 4 years ago

@benjamingr good point, @spion proposed proxy trapping constructors as well here, I thought I had tried and it didn't work (typewise), but reading back that conclusion is nowhere, so I have to re-investigate that :)

mweststrate commented 4 years ago

@urugator good reminder, I think we should definitely enable strict mode in this major as well

urugator commented 4 years ago

Did a quick naive test in chrome (results in comments):

<script>
  // bound
  function onClick() {
    console.log('start');
    const o = {};
    class Clazz {
      constructor() {
        this.fn = this.fn.bind(this);
      }
      fn(arg1, arg2, arg3) {
        console.log(arg1, arg2, arg3);
      }
    }
    for (let i = 0; i < 100 * 1000; i++) {
      o[i] = new Clazz();
    }
    console.log('end');
    window.document.getElementById("status").textContent = "DONE";
  }
  /*
  JS Heap by iterations:
  10k = 1.6MB
  100k = 6.6MB
  1M = 52MB
  10M = 519MB
  */
</script>

<button onclick="onClick()">Run</button>
<div id="status"></div>

<script>
  // prototype
  function onClick() {
    console.log('start');
    const o = {};
    class Clazz {
      fn(arg1, arg2, arg3) {
        console.log(arg1, arg2, arg3);
      }
    }
    for (let i = 0; i < 100 * 1000; i++) {
      o[i] = new Clazz();
    }
    console.log('end');
    window.document.getElementById("status").textContent = "DONE";
  }
   /*
  JS Heap by iterations:
  10k = 1.5MB
  100k = 3.2MB
  1M = 25MB
  10M = 263MB
  */
</script>

<button onclick="onClick()">Run</button>
<div id="status"></div>
seivan commented 4 years ago

If I can give my 2 cents,

I like using classes for my stores, and I like things being predictable which means I always use arrow functions for my stores so I know what this means. I also love using decorators, but only use it for Mobx (as of now), and I actually quite like it.

Though I understand the reasoning for removing them, I only hope whatever the alternative is, can be just as non-intrusive as decorators currently are for me as a consumer .

I would love something like this: 

class Doubler {
  value = observable(1)
  implicitValue = 1

  double = computed(() =>  {
    return this.value * 2
  })

  increment = action(() =>  {
    this.value++
  })

  betterIncrement = (with: number = 1) =>  {
    this.implicitValue += with
  })

  constructor() {
   //Should this get called before values are assigned in the constructor or after? 
    autoInitializeObservables(this, {only: ["increment", "implicitValue"]})
  }
}

Since the other members are explicitly set as actions and observables, the initializer would just do it for the ones defined. Would love an only and/or except version of the autoInitializeObservables.

Now seeing this issue, I'll be moving towards this API for existing code, as the writing is on the wall for decorators. If there's shaky support for it, it's better not to use it.

decorate(Doubler, {
   value: observable
    implicitValue: observable,
    betterIncrement: action,
    betterIncrement: action,
})

The downside is, that it is error prone.

Also curious, in the alternative API what type would value have in this case?

I like it that Mobx hides the actual types when using Typescript letting me work with what I am expecting instead of wrapping everything in Observables<T>.

I generally prefer explicitness, but it's a nice way of not being obtrusive and letting me refactor when I need to, or even remove Mobx as my components wouldn't rely on them. I've always appreciate that, and I'd hate to see that go away.

However, in classes, we typically want more fine grained control, and I think the above should be avoided typically.

I don't agree, I only use classes for Mobx, and with that in mind it's because I want thing to be observables out of the box without me thinking about manual input, though I understand the need for granular control, I just hope it's not enforced when using classes.

Out of curiosity, will setters automatically be converted to actions if the getter is defined as it currently is?

vivainio commented 4 years ago

Also consider new package name for the decorator-less api (mobx-core?). Full break from the existing API often deserves it.

tkrotoff commented 4 years ago
  1. Support proxy and non-proxy in the same version

I wouldn't do that. IE is the only platform without Proxy support.

IE global market share is currently at 1.71%

Even Bootstrap v5 drops IE: https://github.com/twbs/bootstrap/pull/30377

By the time MobX 6 is released, IE will be irrelevant.

elektronik2k5 commented 4 years ago

I wouldn't do that. IE is the only platform without Proxy support.

Unfortunately, that's only true for the Web. But JS and mobx run on other platforms too. Namely, React Native is still bundled with an old version of JavaScriptCore which doesn't support proxies. :cry: Furthermore, Facebook's new JS runtime "Hermes" for Android also doesn't support proxies, yet.

And those are just the examples I know of. There are more esoteric JS runtimes out in the wild, which we should support.

BTW, as much as I'd love to ditch IE (which I personally refused to work on for the last several years), that's just not an option for a LOT of corporate/legacy codebases.

seivan commented 4 years ago

I think it's great if Mobx6 is going to support both ES5 and ES6, as if it's combining the implementation of Mobx4 and Mobx5 in a single package.

We target Mobx4 because we want to support IE11, but we're doing two outputs per target, and support both ES5 and ES2015 (ES6) via <script nomodule> and <script type="module">

My question is, how can we use Mobx6, and pick Proxy for ES2015 bundle and old Mobx4 implementation (observable?) for ES5 outputs, any docs or ideas prepared for this?

mweststrate commented 4 years ago

@seivan yes, that would be a matter of calling something like enableES5Support() in one of the bundles, and not in the other one. That could be done in an entrypoint, if you have different ones, or behind a environment variable that is compiled away, depending your setup.

mweststrate commented 4 years ago

Ok, I think I'm leaning more towards the alternative proposal after giving this some more thought for a couple of reasons:

  1. sharing methods and thereby reduced memory footprint for actions and computes
  2. no subtleties in (not) binding context nor in types
  3. less noisy syntactically
  4. easier to make compatible with contemporary and future decorator implementations

The biggest downside still being: no co-location

Thinking about the API, two ideas:

A decorator map, so it would look like

constructor () {
  makeObservable(this, { 
    field: observable,
    field2: observable.shallow,
    method: action,
  }
)

Calling just makeObservable(this) would automatically mark all fields with the best fitting thing

The downside of this approach is that I don't know how to do "auto" decorating with some exceptions. An additional API like makeAutoObservable(this, exceptionsMap?) looks a bit ugly, although it is not too bad

A fluent interface

constructor() {
  makeObservable(this)
    .observable("field1", "field2")
    .observable.shallow("field3")
    .action("method")
}

Downside is that strings always look ugly, despite being type-checked. Upside is that it is less verbose for large classes as observable is not repeated over and over again for each member (and requires no additional imports).

Exceptions are easy as well, for example if we don't want to mark field1 as observable, field2 should be shallow, and all others should be observables that could be expressed like:

constructor() {
  makeObservable(this)
   .ignore('field1')
   .observable.shallow('field2')
   .auto() //whatever seems fit for all the others
}

No auto decorating for subclassed classes

A subtlety is that we probably should forbid automatically decorating member if there is a super class, as we can't detect which members come from this class and which one from the super, and marking all fields could break internal assumptions of the super class.

danielkcz commented 4 years ago

Downside is that strings always look ugly, despite being type-checked.

Um, your first example has strings too, just quotes are omitted from object keys ;) But yea, TypeScript would help a lot with that. I like the first variant more, fluent feels way too noisy. And exceptions are easy in the first approach too...

  makeObservable(this, { 
    field: ignore,
    field2: observable.shallow,
    method: action,
  }

No auto decorating for subclassed classes

Isn't that a good thing? Inheritance can be so confusing. The composition usually works better.

szagi3891 commented 4 years ago

@mweststrate Experiment :) And what if you tried to go down a level?


type Getter<T> = () => T;

const firstName: MobxValue<string> = new MobxValue("John");
const secondName: MobxValue<string> = new MobxValue("Black");
const full: MobxValue<boolean> = new MobxValue(false);

const computedFullName: MobxValue<string> = computed(
    firstName,
    secondName,
    function(getFirstName: Getter<string>, getSecondName: Getter<string>): string {
        const name = getFirstName();             //subscribe for firstName
        const surname = getSecondName();         //subscribe for secondName
        return `${name} ${surname}`;
    });

const computedForDisplay: MobxValue<string> = computed(
    full,
    firstName,
    computedFullName,
    function(getFull: Getter<boolean>, getFirstName: Getter<string>, getFullName: Getter<string>): string {
        const full = getFull();

        if (full === true) {
            return getFullName();           //subscriptions for "full" and "computedFullName"
        }

        return getFirstName();              //subscriptions for "full" and "firstName"
    }
);

console.info(computedForDisplay.get());     //should show "John"
full.setValue(true);
console.info(computedForDisplay.get());     //should show "John Black"

In such api you can easily share combinator functions without unnecessary memory allocation.

danielkcz commented 4 years ago

@szagi3891 How is that related to classes? There are obviously cleaner ways when you go functional, but some people prefer classes for reflection and stuff.

urugator commented 4 years ago

sharing methods and thereby reduced memory footprint for actions and computes

Imo unless someone shows some really concerning numbers, memory consumptions shouldn't be a major argument. I don't want to give that test above much relevancy, but it shows that if you have under 10k functions (which is quite a lot already) there is no noticable difference and for anything above it's 2x more memory. With 10 milion times more functions, the memory doesn't grow milion times more, but only twice as much. But even if it would be more, it doesn't matter. If a memory is a real concern for you, you won't be creating thousands of instances in the first place. Actually you may not even use mobx. If your app takes too much memory it's not because mobx autobinds functions, but because it's poorly designed. On the other hand keeping functions on the object is practical and can simplify a lot I believe (impl wise and ux wise).

"auto" decorating with some exceptions

ignore decorator. I think we will need an internal "decorator" marking the field ignored anyway. So why not exposing it and keeping it simple/transparent.

A fluent interface

The pattern is used to help with building/configuring objects (often immutable). But the premise is that the manual configuration should be really rarely needed (due to sane automatic defaults). Therefore I don't think it adds much of a value.

we can't detect which members come from this class and which one from the super, and marking all fields could break internal assumptions of the super class.

If everything is observable/action/computed by default, then we know which fields must not be made observable, because they must have been marked as ignored in superclass. EDIT: That's the whole idea - by default everything is under our control, therefore we can make various assumptions and even eg throw on places we couldn't before...

mweststrate commented 4 years ago

Yeah I don't think we need any changes in dealing with non-classes. Also in mechanisms like the above getting this and self referencing types is really tricky, that is a recurring painpoint in MST as well.

On Sat, Apr 4, 2020 at 4:11 PM Daniel K. notifications@github.com wrote:

@szagi3891 https://github.com/szagi3891 How is that related to classes? There are obviously cleaner ways when you go functionals, but some people prefer classes for reflection and stuff.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx/issues/2325#issuecomment-609042910, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAN4NBDXNEUEI5LWY4VPZELRK5EZ5ANCNFSM4LZQ2B5Q .

szagi3891 commented 4 years ago 
constructor () {
  makeObservable(this, { 
    field: observable,
    field2: observable.shallow,
    method: action,
  }
)

I also like this version more.

Is it possible to set TS to check that all fields that are not readonly must be marked as observable? It would also be nice for TS to check that we marked a field readonly with an observer and treat it as an error.

Imo unless someone shows some really concerning numbers, memory consumptions shouldn't be a major argument.

I had very big problems with memory usage. I also had a problem with decorators. The application I use must create huge amounts of models and just initializing decorators took a lot of time.

spion commented 4 years ago

I don't like the explicit specification of observables. It was already easy enough to forget the @observable decorator for fields that need to be observable (a very common mistake, happens all the time). It is going to be even easier to forget if you have to specify it in the constructor, potentially several dozen lines away from the declaration of the properties, getters and methods.

autoInitializeObservables with opt-out is much better, but memory consumption for computers is a potential issue, and higher control for refs/shallow/deep observables is another. But I think it's going to end up very usable, potentially even better than explicit @observable decorators. Plus, it can also be used as a class-level decorator that does constructor trapping via proxy or something similar.

My question is, what problem are we trying to solve here? If it's the field initializers issue, we can still use decorators as pure metadata which will then be read by autoInitializeObservables. Decorators as pure metadata are not likely to go away and are the most likely subset that could get eventually standardized.

mweststrate commented 4 years ago

Deserves a more extensive answer, but I think the benefit of the alternative is that it is easy to keep supporting both decorators (just not as the default advertised approach), so all these 3 approaches would be valid:

class Test {
  field = 3

  constructor() {
    makeObservable(this, { field: observable }) 
  }
}

// or, leverage meta data emitted by decorators

class Test {
  field = 3

  constructor() {
    // doesn't need a second argument, but would accept a second argument with _exceptions_
    makeAutoObservable(this) 
  }
}

// or, leverage auto observable

class Test {
  @observable
  field = 3

  constructor() {
    // without the second argument, see if we have meta data or die
    makeObservable(this) 
  }
}

This still solves the problem of having a small bundle as the meta data decorators don't need to provide a semantic implementation, and babel has a plugin to support TS style decorators as well

I still don't see having proxies as having a clear benefit over a constructor, I think wrapping a class is a lot more 'advanced' for many JS devs than having a constructor

olee commented 4 years ago

I also like this option a lot. I think it's a good idea to use decorators only for specifying metadata, as this will always be possible no matter the final decorators implementation will look like. However I would like to propose some slight changes:

class TestAutoObservable {
  observableField = 3

  constructor() {
    // 2nd argument true => auto observable
    makeObservable(this, true)
  }
}

class TestAutoObservableAdvanced {
  observableField = 3

  unobservableField = 3

  constructor() {
    // 2nd argument true with 3rd argument options => auto observable with exclusions / adjustments
    makeObservable(this, true, {
      unobservableField: false, // false could be an alternative to a special "ignore" value
    })
  }
}

class TestManual {
  observableField = 3

  unobservableField = 3

  constructor() {
    // 2nd argument options => manual observable
    makeObservable(this, {
      observableField: observable,
    })
  }
}

class TestDecorator {
  @observable
  observableField = 3

  unobservableField = 3

  constructor() {
    // only manual decorator support (mixed decorators / options could also be possible)
    makeObservable(this);
    // makeDecoratorObservable(this)
  }
}

class TestDecoratorAuto {
  observableField = 3

  @ignore
  unobservableField = 3

  constructor() {
    // 2nd argument true => auto observable with decorators to ignore fields
    makeObservable(this, true);
    // makeDecoratorObservable(this, true)
  }
}

This implementation would only require a single function with an easy to understand API.

Another idea that might be useful would be to put the decorator support into a separate package (or separate import like mobx/decorator) which would provide a function like makeDecoratorObservable. This function would then be a higher order function on makeObservable which would take the provided (or empty) options and fill it with the decorator metadata from the class. This would allow completely excluding any code regarding decorator support from bundles which do not need it.

EDIT: One more idea: I think it should also be possible to provide a function like makeClassObservable, which would take a class instead of this as the first argument with the remaining signature being the same as makeObservable. This function would then return a subclass where the constructor automatically calls makeObservable after the call to super:

export const TestAutoObservable = makeClassObservable(class {
  observableField = 3
  actionMethod() {
    // do stuff
  }
}, true)
danielkcz commented 4 years ago

@olee Sorry, but how is makeObservable(this, true, {...}) better than makeAutoObservable(this)? Not only it's longer, but boolean arguments are so confusing because people unfamiliar with API have to look up what it means. When you have named variant that clearly expresses difference, it's so much better DX-wise imo.

Let's not do that just to have a single universal overloaded function, that's the wrong reason. I like your makeClassObservable if that's possible, but once again rather than boolean arg I would go with makeClassAutoObservable or even makeClassObservable(class {}, { auto: true }) is better.

olee commented 4 years ago

Yeah when you say it like that, it definitely makes sense (that's also the reason why I added it just as an addition.

What about the other points?