mobxjs / mobx-react

React bindings for MobX
https://mobx.js.org/react-integration.html
MIT License
4.85k stars 349 forks source link

mobx-react 6 #640

Closed mweststrate closed 5 years ago

mweststrate commented 5 years ago

Things to be addressed:

danielkcz commented 5 years ago

Hm, so you want to ditch the mobx-react-lite eventually? It probably should merge together at some point, but wouldn't it be too convoluted trying to support classes and hooks in one package?

danielkcz commented 5 years ago

Also, I convinced that we should teach people how to use React Context instead of including it in the package. For once it's super easy to do it and it doesn't seem it gives much of the benefit having it here. On the contrary, it would be harder to have proper typing for it.

mweststrate commented 5 years ago

@FredyC definitely not, the question is more, merge it into mobx-react, or make it be the next mobx-react (dropping propTypes, context, provider / inject and such). But I doubt we can loose the latter on the short term, I think most people still prefer the mobx-react api over React's own context api. So, yeah I am basically having the same questions as you have :)

danielkcz commented 5 years ago

Switching mobx-react to a new context API would make a bit nicer and smaller repo code for sure. However, as long as people will be using inject, they won't really know there is a different tech internally.

Personally, I don't see such a huge problem including one extra Provider in the tree to cover the transition. This isn't a first package suffering from that issue. But we should definitely make some migration guide explaning gradual adoption.

urugator commented 5 years ago

I would personally like to see prop-types/inject/etc in their own seperate packages.

danielkcz commented 5 years ago

@urugator Can you give some sensible reason why? I mean why is inject better for you instead of using context directly? Check this explanation if you haven't yet. Keep in mind that you cannot use decorators with functional components.

urugator commented 5 years ago

Where do I claim that I use inject or that it's better? Let me reformulate: If there should be a custom inject utility, it should be in it's own seperate package (maintained by whoever needs it for whatever reason).

danielkcz commented 5 years ago

😄 Yea, I would probably agree, but that's not really the point of this issue which is more about migration strategy between two packages. Completely removing Provider/inject from mobx-react would effectively block anyone using it in existing projects.

I think that V6 should be more about those last two points from the checklist and if Provider gets the revamp along the way, that's more of the bonus.

JabX commented 5 years ago

While mobx-react-lite (or some other package) can keep the customs MobX hooks, at the very least both observer implementations should be merged into mobx-react's, since right now we can't use any hook in a "regular" observer function component (mobx-react wraps the function in a class).

Today if I want to start using hooks in my application I have to use both mobx-react and mobx-react-lite and use the correct observer for each use case, which is far from being ideal.

danielkcz commented 5 years ago

@JabX And you want to use classes for a new code? 😮 Cannot imagine why really, but I think it's a general direction of a whole React to leave this dark time period behind :) For any new component you can use lite package and eventually refactor class components.

I think that the main goal of mobx-react at this point should be to support class components as long as it's necessary while still being relevant in regards to the concurrent mode and other future features.

I could imagine that mobx-react could be a proxy for mobx-react-lite at this point. When it detects it's a functional component (does not have a constructor property), it would use lite observer directly, otherwise fallback to a current solution. It can also reexport all other hooks just for convenience so we don't need to use multiple imports.

In some long term, I guess we are talking years here, the mobx-react could ditch classes completely and become what the lite package is at this moment.

JabX commented 5 years ago

@FredyC At the very least I'd like to use hook-based components next to existing class-based components without having to use two separate implementation of the same function (observer).

And to answer your question : yes I'll probably keep using classes for new code, at least at some places where it makes sense to do so (large components, and even if it's frowned upon it's still very convenient to just extend an existing class component to override one method and keep the rest as is).

I don't really like the hooks API because it feels hacky at best, but I like what useEffect and useContext can do (this is so much better than what we have for classes), so I'll probably use them quite a bit. I wish the React team would backport some hooks features to classes, but anyway that's not really the question here.

matsilva commented 5 years ago
  • plan strategy for hook based components (currently; use <Observer> or mobx-react-lite)

Does this strategy include a guide to help folks migrate?

mweststrate commented 5 years ago

I think the natural behavior consumers expect is import { observer } from "mobx-react" supports both hooks and classes (they are far from gone yet!). Whether that implementation is merged in or delegated is I think quite indifferent.

I think merging in (mostly to not duplicate facilities around it, such as devtools), and at the same time moving non-essentials to separate package (like proptypes (recommend: TypeScript) and inject (recommend: React.createContext)) is quite a viable approach

danielkcz commented 5 years ago

@mweststrate Instead of merging I would rather go the way of dependency and check for a functional component to decide which implementation to use. Having two separate copies of the same code sounds kinda bad imo.

mweststrate commented 5 years ago

In both cases there are two implementations. The advantages of merging is that it is easier to port fixes from one to the other. The advantage of not merging is that people an opt to use the function version (lite) only

danielkcz commented 5 years ago

I am not sure I understand. How is including lite version as dependency making it two implementations? There will be a simple condition in current observer which will proxy to lite. That's it. Am I missing something?

The advantage of not merging is that people an opt to use the function version (lite) only

I am one of those people :) I have fully migrated to lite and I don't want to bundle the garbage coming from class version again.

mweststrate commented 5 years ago

How is including lite version as dependency making it two implementations?

There will be for now two implementations be maintained in either case; one based on hooks and one based on classes. That it is in a separate package, doesn't mean it doesn't need maintenance ;-)

I am one of those people :) I have fully migrated to lite and I don't want to bundle the garbage coming from class version again.

If all other features are removed from the package, except the class support, the difference in package size will probably be < 1KB. Which might not be worth the confusion. Especially because adding devtools and other chores such as static hoisting, forwardRef support, etc. to the lite version, might result in that very same 1KB being lost in duplication if both are included (which happens always in the non-lite version).

An interesting question is also the opposite problem: what if people are not interested in hooks for their code base? They will still get all the hooks (although these are tree-shakeable)

(for all clarity, I don't have a strong opinion about all this yet, just elaborating on pro's and con's)

urugator commented 5 years ago

two implementations?

One that uses hook, one that uses classes. Merged means that code for both is in one package. Delegated means the code for hooks is imported.

Imho most of the people will expect the observer to work with both (func and classes).

what if people are not interested in hooks for their code base

I suppose the hooks impl is even smaller. Obviously we could end up with something like mobx-react-hook and mobx-react-class with mobx-react being the thin layer on the top of them. Would be ideal (?), dunno about maintenance cost.

tree-shakeable

If possible, please don't rely on tree-shaking, it's not always available (without additional costs).

Does hook version support reactive props? If not, will it be removed from class version?

danielkcz commented 5 years ago

Ok, let's drop theoretical argument about size savings, it's probably irrelevant at this point.

I certainly do not mind maintaining lite version separately as I doubt I will stop using MobX in a foreseeable future and at the same time I won't be working with class-based project anytime soon (if ever) so I don't care about this package that much ;) Besides, I have spent some time on a lite package, especially rewriting all those tests and don't like the idea of throwing that away so soon.

@urugator

Imho most of the people will expect the observer to work with both (func and classes).

Personally, when I was migrating it was actually helpful to see where the old package is still used and I should refactor since it was gradual adoption. So I am not entirely convinced it's such a huge problem to have two independent packages each taking care of its domain properly.

In theory, we could invent some heuristics here which will detect if lite is available and use it otherwise fallback to the current implementation. It could as well check if React 16.8 is installed and recommend the lite for a functional component. Never tried this before, would it be viable? Would there be some negative impact on a production code?

If this a dumb idea in overall then I am strongly inclined about including lite package as a dependency, at least for the time being.

danielkcz commented 5 years ago

@mweststrate Btw, add the TypeScript consideration to the list. This package is still just a JavaScript.

JabX commented 5 years ago

@FredyC

Personally, when I was migrating it was actually helpful to see where the old package is still used and I should refactor since it was gradual adoption. So I am not entirely convinced it's such a huge problem to have two independent packages each taking care of its domain properly.

Hooks-based components are not here to replace class components, they're here to provide a new (and arguably better, but the jury's still out on that one I guess) way to write stateful/"effectul" components. It will probably become the preferred way to write components in the future, but it will never outright replace class components, just like React.createClass still exists today and is by the way still supported by mobx-react (though I don't think it does a lot of stuff to explicitly support them since they are still classes).

No one will ever understand why or accept that observer from mobx-react does not work on hook-based components while it is supposed to and should work on all kinds of components. I don't mind splitting side features into their own packages (like the hooks from mobx-react-lite or Provider/inject), but, and as everybody else on the thread also said, there should only be one observer implementation.

danielkcz commented 5 years ago

I see one major problem here. The lite observer is written using hooks. It requires React 16.8+. And that means mobx-react@6 would require such a version of React as well. I am sure there are projects still running React15 or any pre-hook version and they won't / can't upgrade just yet.

I don't think people would be very happy about being forced to a specific React version with such a major package. The mobx-react-lite can afford that as it's built on top of bleeding edge tech. If people use that package they are agreeing with that aspect.

Ultimately it would mean we would need to maintain V5 a V6 in regards to any bug fixes and features and I don't think that's what we want here.

urugator commented 5 years ago

It requires React 16.8+

Since most of the planned changes directly relates to new react features (even ctx, error boundary etc are relatively new), it seems logical to require upgrading react together with mobx-react.

need to maintain V5 a V6

If mobx-react would simply reexport the individual packages (mobx-react-hook/mobx-react-class) as suggested than the maintenance should be pretty minimal I suppose. V5 would be a proxy for mobx-react-class (or -legacy or whatever) V6 would be a proxy for both with feature detection, requiring new react. (Actually I think it would be V6 and V7, due to other changes?)

Or make hook package an optional dep?

danielkcz commented 5 years ago

I would suggest names like mobx-react-legacy and keep mobx-react-lite, but otherwise, I like this plan. Feels like a reasonable way on how to get out of this mess gracefully.

mweststrate commented 5 years ago

@FredyC, @urugator I think we should bump minimum required version to 16.8 indeed. For everything else the current version can be used as is.

urugator commented 5 years ago

TBH I am not a fan of mobx-react-lite as it seems a bit bloated already (useComputed/useDisposable) and I am afraid it will continue to grow. Imho there should be just observer/useObserver, everything else is expendable and opinionated. I don't mind having an utility/commons package of sorts, but it shouldn't be part of the "core". Perhaps even the package should be named just mobx(-react)-observer or something.

I don't like generic terms (legacy and lite) as it gives no clue to the user for what, why or when it should be used. It also isn't future proof as another rewrite could generate something like even-lighter or not-so-legacy. Would it be weird to have a minimal react version as part of the package name? Like mobx-react-168?

danielkcz commented 5 years ago

TBH I am not a fan of mobx-react-lite as it seems a bit bloated already (useComputed/useDisposable) and I am afraid it will continue to grow.

Well, mobx-react also includes Provider/inject while it's not MobX related. Nobody is forcing you to use any extra utilities exposed by a package. Let's not over-package it please as it usually leads to compatibility issues between packages and it's annoying to tackle it properly. Do you have some actual argument why it's wrong to have it in a single package? And let's put size matters aside really.

We certainly need better docs, an ideally single site for all packages. Currently, there is no such site even for mobx-react. @mweststrate Would add to the list as well?

urugator commented 5 years ago

includes Provider/inject

unfortunately

forcing you to use any extra utilities

User is confronted with the extra complexity, whether he uses it or not (when dealing with docs/sources/debuging/other users etc).

actual argument

I don't want to get into long polemics, so a burst of thoughts in few points, whether it makes sense or not:

danielkcz commented 5 years ago

@urugator With such reasoning, you could as well split React package into numerous small ones. For examples why react/test-utils and not as a separate package? Or a whole SSR logic could be separate. If you want to bring some real reason on the table, please open issue in mobx-react-lite repo, this is heavy OT.

urugator commented 5 years ago

Afaik react is organized into individually importable subpackages. Some non-essential things like prop-types, element factories, etc moved completely away. The scope of react is fundamentally different and so is the scope of included features. Under certain circumstances SSR or test utils are necessities, the same cannot be said about useComputed/useDisposer and similar. Also the react modularization is an ongoing process, so who knows what the future holds.
Perhaps more relevant comparison would be: redux, redux-connect, redux-thunk, reselect ... or koa, koa-body, koa-send, koa-compose...

mweststrate commented 5 years ago

@FredyC @andykog the biggest open issue currently are the DevTools.

The devtools have basically three features:

  1. Log mobx actions / reactions, etc
  2. Report rendering speed
  3. Show dependency trees

The problem introduced with hooks is that there is no clear / official relation anymore between DOMNodes and hooks which we can easily intercept. (FindDOMNode is deprecated, and useObserver could in principle be used multiple times in the same component. Function components don't have a real reference to their backing instance with which we could associate meta data etc).

So the impact of this is as follows:

  1. Keeps working, spy is powered entirely by mobx, not mobx-react
  2. Report rendering speed, breaks, but is nowadays a standard feature of the official react devtools, so is not really needed any more
  3. Here we have a few options
    1. Write some complicated hacks, using some not officially exposed react internals, and try to restore current functionality (select component in DOM, see tree view)
    2. add some param to observer, or provide a useTrace hook that (logs?) the dependency tree to the console
    3. in DEV mode, store the dependency tree as object as 'state' in the useObserver hook, so that people can inspect it from the react devtools, somewhere in the "hooks" section

I think 1 is still the coolest interaction. The benefit of 3 is that it removes a lot of code, and works without installing devtools

screenshot from 2019-02-14 13-43-19 :

mweststrate commented 5 years ago

screenshot from 2019-02-14 14-50-29

Doesn't look nice :(. (when using object an object, it's not expandable, so it has to be stringified). Also, this own't work for non-function components.

danielkcz commented 5 years ago

As strange as it may sound, I have never used any sort of debugging with MobX, so I am not even sure what to expect from it :/ Number 3 sounds best to have some debugging (even limited) options out of the box available.

andykog commented 5 years ago

@mweststrate you actually mean not dependency trees, but rather DOM/React/Observers nodes tree, right?

  1. Write some complicated hacks, using some not officially exposed react internals, and try to restore current functionality (select component in DOM, see tree view)

I'm worried that we don't have enough maintenance capacity to constantly align with react internal stuff

2. add some param to observer, or provide a useTrace hook that (logs?) the dependency tree to the console

Might work, needs investigation

3. in DEV mode, store the dependency tree as object as 'state' in the useObserver hook, so that people can inspect it from the react devtools, somewhere in the "hooks" section

Maybe ok for a backup plan

mweststrate commented 5 years ago

Hey folks, here is an idea for a more radical change of V6, on top of the current changes that are already pending, see #646

The important, additional changes in this branch:

  1. mobx-react doesn't have it's own observer implementation anymore! Instead, it just wraps the render method of class based observers in <Observer/>
  2. Threw away everything that relates to DevTools. (probably should put the api back and print a warning, this is just to get the idea)

Noticable changes

  1. Significant reduction in the size of the library

  2. class based observer components now always show an additional <Observer> component in the tree, which one might consider a downside. See screenshot below.

  3. no support in the for own MobX devtools anymore, but

    1. event spy remains working, as that is coming from mobx
    2. render speed visualization is now build into the react devtools
    3. there is no nice visualization of component deps anymore :cry: . But, the good news is that a lesser visualized dep tree is visible in the inspector panel of the standard React devtools
  4. PureComponent is now supported. Rather, it is now actually recommended!

  5. componentWillReact hook is no longer supported

  6. Not verified, but I think there is no need anymore to ship separate React-Native builds, since the difference was only devtools support, IIRC.

  7. Except for render, we don't touch patch life cycle methods anymore, so that means that trouble with componentWillMount: () => ... etc is solved as well now!

Further work (even wilder ideas):

  1. we could propTypes, Provider / inject and disposeOnUnmount together in mobx-react-utils as suggested by @urugator.
  2. If we do that, than we could also add class support to mobx-react-lite, and have it replace mobx-react
  3. The make-state-and-props-observable hack is still in there... would be cool to get rid of it, but it is really hard to explain to the unaware why these cases don't work...

screenshot from 2019-02-14 20-58-15

cc @xaviergonz @k-g-a @urugator @FredyC @andykog

xaviergonz commented 5 years ago
  1. Do you mean users are supposed to put it manually or that the new @observer will do that internally? (I guess the last, if so sounds good to me)
  2. To be honest I only used the devtools to see when a component was being re-rendered, which I think react dev-tools can do as well

Changes:

  1. Yay
  2. Personally I don't mind
  3. Never used those (except the render speed/rerender, which is also now in react-devtools, and except I think the dependencies thing once or twice), but I don't think it is critical
  4. More power to the people :) (though the refactor might be a bit annoying, but it's ok)
  5. Never used it
  6. Haven't used react native
  7. disposeOnUnmount still needs to touch those, but yeah, it could be moved to mobx-react-utils. As for provider/inject, I think react context is a better alternative really (don't see a reason to keep it at all), and propTypes, never used it since I use TS, but I see it as worthy to be moved into react-utils as well

3. The make-state-and-props-observable hack is still in there... would be cool to get rid of it, but it is really hard to explain to the unaware why these cases don't work...

I don't remember right now, but do those only touch the render part as well?

mweststrate commented 5 years ago

@xaviergonz for question 1, yeah, the last!

For 3: It works quite consistently. But monkey patching the React instance adminstration is still a yuki thing I would gladly get rid of. Somehow :smile:

JabX commented 5 years ago

Not sure what dropping the current observer implementation achieves except dropping a bunch of stuff that is occasionally used/useful. Skimming through the proposed changes, it seems that most of the benefits would come from dropping devtools support. As already said before, that makes sense because devtools represent a lot of code that isn't that useful anymore (regular MobX and React tools already get us pretty far) and rely on deprecated stuff that won't be replaced (findDOMNode mostly I guess?). Maybe we could stop there? Without devtools, the hooks implementation can be kept as-is and is simple enough to be standalone (as in not sharing code with the class one, as opposed to being a different import).

If stuff like disposeOnUnmount gets moved to a new package mobx-react-utils, where should the various hooks from mobx-react-lite live? It seems easier to me to put everything into mobx-react and eventually let tree-shaking to its job for the unused bits, if necessary.

danielkcz commented 5 years ago

2. If we do that, than we could also add class support to mobx-react-lite, and have it replace mobx-react

I don't follow why to even consider such foolishness :) It's the LITE package, it's not meant to be backward compatible. It would only create confusion "what package should I pick?". If someone needs to support classes, let's have the mobx-react for that purpose.

Please let's not create such havoc. The lite package got its 1.0.0 version just recently and I don't want to confuse people that they need to use different package now.

If stuff like disposeOnUnmount gets moved to a new package mobx-react-utils, where should the various hooks from mobx-react-lite live?

I think I would like to keep additional hooks in that package, at least for now. I definitely don't like the idea of having bunch of small packages. As I said above, it's hell to maintain it properly and ensure compatibility.

urugator commented 5 years ago
  1. <Observer/>

It also makes https://github.com/mobxjs/mobx/pull/1811 impossible to implement, because we don't have an access to reaction in SCU, correct? EDIT: or do we via ref?

4. PureComponent is now supported. Rather, it is now actually recommended!

Use of observer on non-pure component still results in "pure" behavior. That may be confusing. I think we should rather use default/user-defined SCU on non-pure components. A few users already got surprised by implicitely pure behavior and the inability to define own SCU. It made sense back then when PureComponent/memo wasn't an option, not now imo. While mostly useful, it's still an optimization and shouldn't be forced. It's another code reduction (of practically duplicated code). I think it removes the need for forwardRef option for func component and therefore for the option object as a whole. If memo(observer(() => {})) is too verbose, pureObserver/memoObserver could be provided (without options, aplicable to func comp only), personally I would leave it to userland.

3. The make-state-and-props-observable hack is still in there

We could try to remove it and re-integrate only if actually requested. With the adoption of hooks and increased usage of functional components, there is a possible shift from class level computed/reaction to useMemo/useEffect. Hypotetical useComputed/useReaction should accept non-observable deps (props/state) as [x,y,..] similary to others.

  1. we could propTypes, Provider / inject and disposeOnUnmount together in mobx-react-utils

Imo propTypes should be seperate. It's useless for Mobx>4 and it can be used as devel dep only.

mweststrate commented 5 years ago

I don't think #1811 is worth pursuing, it is a mental model that deviates from how MobX normally works, the cases where it is really beneficial are I think not that common, and really hard to recognize. Simply from teaching / confusing perspective I think it is not a good idea to introduce a second tracking model.

I think since the introduction of <Observer> there is no reason to support observer + custom sCU anymore. If sCU is that important, just split things up. It is probably an indication that you are mixing mobx and non-mobx data sources in a single component anyway.

The reason for defaulting to memo / Pure / sCU is that mobx-react's message basically is that components become independently rendering things, for an optimized experience out of the box. Not doing so basically reverts back to React's default opt-in optimizations, forcing people to think about optimization again, something we graciously took from their hands so far. Also, in the past this very rarely have ever raised questions / issues from what I've seen in workshops, the issue tracker etc.

We could try to remove it and re-integrate only if actually requested.

The problem is that it will break current components, but the bugs can only be detected at runtime. And it will be requested, as this feature has only been added after much request (I really didn't like it from a technical perspective, although from a conceptual perspective it makes sense)

It's useless for Mobx>4 and it can be used as devel dep only.

I think they are still almost as useful as before, mostly to assert that the props are actually observables if your component relies on that for change detection.

I think separating packages are overrated; it complicates maintenance, not just package management, which can be automated from a mono repo, but also documentation get's fragmented over the place, and asking questions to install 5 packages is just unnecessary burden imho in the age of tree-shaking. I think the main reason for splitting packages is difference in domain, or the API surface getting too big.

How does the following split between packages sound (see below)?

The alternative sane split I see is to add class support to mobx-react-lite, move the other 3 features to a separate package mobx-react-utils. Rename mobx-react-lite -> mobx-react and call it a day :). The benefit of the below split is however that we could use to not expose some (or all) of the hooks, if we don't consider them idiomatic yet. (Didn't put any thought into this yet)


Choosing your version

There are currently two actively maintained versions of mobx-react:

NPM Version Supported React versions Supports hook based components
v6 16.8.0 and higher Yes
v5 0.13 and higher No, but it is possible to use <Observer> sections inside hook based components

The V5 documentation can be found in the README_v5.

Version 6 is a repackage of the mobx-react-lite package + following features from the mobx-react@5 package added:

  1. Support for class based components for observer and @observer
  2. Provider / inject to pass stores around (but consider to use React.createContext instead)
  3. PropTypes to describe observable based property checkers (but consider to use TypeScript instead)
  4. The disposeOnUnmount utility / decorator to easily clean up resources such as reactions created in your class based components.

EDIT: forgot to address two other issues raised

danielkcz commented 5 years ago

The alternative sane split I see is to add class support to mobx-react-lite, move the other 3 features to a separate package mobx-react-utils. Rename mobx-react-lite -> mobx-react and call it a day :). The benefit of the below split is however that we could use to not expose some (or all) of the hooks, if we don't consider them idiomatic yet. (Didn't put any thought into this yet)

I already expressed my feelings about it 😢 I don't want to be tagging along class support when I don't need it. Please, let's not rush these things. We have LITE for early Hooks adopters. Get out mobx-react@6 that will use the LITE underneath to cross the gap for older projects. That's more than enough for now.

Perhaps in the next year, we can reiterate, see where classes are standing and possibly get out V7 that would get rid of them.

urugator commented 5 years ago

It is probably an indication that you are mixing mobx and non-mobx data sources in a single component anyway.

That seems like completely valid usage, especially when we don't use something like connect(mapObservablesToProps). Same argument could be used against "make-state-and-props-observable". Also, what's the use case for class based component (aside of existing code)? Citing from react docs: "There are no Hook equivalents to the uncommon getSnapshotBeforeUpdate and componentDidCatch lifecycles yet, but we plan to add them soon." So what remains? SCU...

reverts back to React's default opt-in optimizations, forcing people to think about optimization again

The assumption that people are forced to think about this is completely wrong. The base react premise is that VDOM and diffing is fast enough, so people shouldn't need to think about this kind of optimization at all (until actually experiencing issues). And notice, it's not always an optimization. There can be a lot of wasted shallow checks and there is no general recommendation to prevent it (perhaps if context would be used exlusively instead of props, but then we can just return false from SCU). Especially if we recommend to slap observer mindlessly to everything.

graciously took from their hands

forcibly

very rarely have ever raised questions / issues

could be also true for an alternative

age of tree-shaking

Tree shaking is not a feature but a workaround. We are continually adding "features" on the top of the things which were introduced as workarounds (transpilation to ESx, merging files together, etc). We make these workarounds necessities and call them a "standard way of doing things", which was never the case. The amount of knowledge to get started with JS is getting out of hand due to it and it makes the JS development a lot more expensive. The amount of tooling we need just to run a project is insane. All that efford and complexity should be put into business logic, where the actual value is. Sorry for OT, but it really bothers me to witness this. I won't comment on it any further.

assert that the props are actually observables

Ah true, haven't realized that.

JabX commented 5 years ago

The base react premise is that VDOM and diffing is fast enough, so people shouldn't need to think about this kind of optimization at all (until actually experiencing issues).

VDOM and diffing might be fast enough, but rendering logic can quickly become expensive if you don't pay attention, especially with lists. I'm not saying that a shallow-compare sCU will solve everything (far from it), but it's always pretty much the first thing you can do to help.

graciously took from their hands

forcibly

very rarely have ever raised questions / issues

could be also true for an alternative

I mean, @mweststrate's post stated that it was a heavily requested feature, so I guess that we have tangible proof that it's the preferred solution

urugator commented 5 years ago

@Jabx

I mean, @mweststrate's post stated that it was a heavily requested feature

He was referring to make-state-and-props-observable hack (or did I miss something?) Requested doesn't automatically mean good, check https://github.com/facebook/react/issues/14501. As mentioned ... PureComponent and memo was not an option before. Benchmarks are only valid proof when it comes to optimization.

VDOM and diffing might be fast enough, but rendering logic can quickly become expensive if you don't pay attention, especially with lists.

You need a fairly large lists or quite complex items to really push it. Try to recollect how much % of all the components in you codebase are such expensive lists/components. The reality is that the pure is really required just on a few specific places. (Not saying it's absolutely without an effect on others) And it's not like you can't use it anymore, it's trivial to do so, just not forced.

mweststrate commented 5 years ago

@urugator

That seems like completely valid usage, especially when we don't use something like connect(mapObservablesToProps).

It is not really anymore. One of the reasons React can't default to pure, is that it doesn't have a store for mutable data that is being passed in, meaning that changes deep inside those objects would always be missed. However MobX does provide an answer to that problem: either a prop is immutable, or it is observable. And in practice that seems to work out really great because that is what one does almost uncousciensly; e.g. a style object is constructed in place during render, (so effectively immutable), and state containing objects (e.g. a user) is based on observables.

But even if you need more control, that is no problem at all: Just write vanilla React components, and combine it with <Observer> for the reactive render. @observer is just a shorthand for <Observer> + pure + reactive props/state. But it can be omitted without problem if you want more control and less convention.

The reason why many people choose for mobx, especially when migrating to it, is because introducing MobX stopped rendering their components like a christmas tree if something high up in the tree changes. This is after all the default behavior of React, but an observer component basically states: "either give me different props, or change some relevant observables, or I won't re-render if my parents change".

The base react premise is that VDOM and diffing is fast enough

The VDOM diffing is generally not the problem, but the step before that; the reconciliation. On slightly larger collections or deeper trees (e.g. 100+ components) this quickly becomes noticable

And notice, it's not always an optimization.

That is true, but, that is only the case if children to always need to re-render if their parent changes. If the components are so closely related, than I think usually those children are not modeled as observer in the first place. But in any case I didn't encouter ever a case where this was an actual problem. (Note that the shallow equal typically operates on a very small set; the props). So in other words, adding sCU by default has been proven to be a valuable optimization, while leaving it out (in the context of a MobX app!) hasn't. But as said, even if this is a problem: their is clean opt-out: Observer.

The amount of tooling we need just to run a project is insane.

Can't see I disagree on this one. But I do think we are moving in the right direction :). It used to be simpler back in the days. But on the other hand, things we shipped in the past were not unlikely to ship 5 versions of jQuery. But yes, on a green field project I would rather leave those features out (Like Provider / inject) or put them somewhere else. But I think the migration path is big enough as it is now, and I'll rather postpone drop-moving those to a next major. But maybe I'm overthinking this one and it wouldn't be that bad...

He was referring to make-state-and-props-observable hack

Correct

As mentioned ... PureComponent and memo was not an option before.

That is not entirely correct imho. PureRenderMixin took care of this, and half of the Redux premise was to solve the sCU problem for you, as you could use that one. And Redux was advocated to be standard companion of React, because without it React wouldn't scale into real life projects performance wise. Unless sCu was handcrafted everywhere, which is error prone and breaks the nice declarative approach of React.

The reality is that the pure is really required just on a few specific places.

It's true that not everywhere where MobX applies optimizations it is strictly needed. (The same holds for really simple @computed values, where the computed caching is probably more expensive than the computation itself). The goal of MobX is to offer good performance balance out of the box. Could you make it faster by very carefully only applying computed / Observer / observable.ref etc in exactly the right places? Definitely! We promise to do a better job at optimization if you don't think about optimization yourself. And we promise to never make it noticeable worse. So MobX applies the conventions that on average gain the most. But can you manually outperform MobX? In many cases probably yes. But the investment would be huge amounts of time.

I think this tweet nicely summarizes it:

kitze-mst

urugator commented 5 years ago

VDOM diffing is generally not the problem

I meant whole rendering process by that, without the time spend on real DOM.

That is not entirely correct imho.

I meant that in relation to Mobx - the need for special utility for functional components and lack of/incompatibility with PureComponent, which is not the case now.

The reason why many people choose for mobx, especially when migrating to it

Migrating from what? To me the main Mobx benefit is automatic subscription management and familiar programming model (mutable state). I've never felt the need to migrate to mobx due to performance issues.

Redux premise was to solve the sCU problem for you

How? Immutable updates must be performed by hand. State normalization must be done manually. Components must be defined pure by user (or connected). Subscriptions are managed by hand (mapStateToProps,etc). None of that is enforced in any way, just recommended. All of that is doable in React alone with the same amount of effort, meaning that React is scalable practically the same way (performance wise).

I don't say that pure by default is bad or that it doesn't make any sense. My claim is that not-pure by default, may not be as bad as one may think, while offering some mentioned benefits (code reduction, api reduction, SCU support, PureComponent/Component difference, SoC). I am convinced that you wouldn't see a radical performance degradation (from usability standpoint) except for few special cases. Use memo/PureComponent everywhere for a good performance balance is still possible as recommendation. But I understand the motiviations for keeping it implicit, we can move on :)

danielkcz commented 5 years ago

I am not going to make some elaborate statements here, merely a note. Consider that with hooks there will be a variant of useObserver and any "pure-ness" doesn't matter there. And the observer itself does not have a memo included. Perhaps it should, I don't know. Feels to me like you are talking mostly about class components which are going to be less and less interesting for people.

As I am growing older (and hopefully wiser) and I am more and more convinced that premature optimization is really bad. I already got burned about abusing useMemo and useCallback several times so I've decided not to optimize until it's actually a problem.

urugator commented 5 years ago

And the observer itself does not have a memo included.

Doesn't? https://github.com/mobxjs/mobx-react-lite/blob/master/src/observer.ts#L41 What am I missing?

danielkcz commented 5 years ago

@urugator Nevermind, you are right :) Anyway, useObserver or Observer is kinda a good way to opt out from this optimization, so perhaps it's not so bad to have a observer that does that.