[Bug?] componentDidUpdate() doesn't trigger after version 6

[RosenTomov]

Like title says, componentDidUpdate() won't trigger since mobx-react version 6.

For example, I want to keep the scroll position to the bottom:

@inject('someStore')
@observer
class SomeList extends React.PureComponent {
...
componentDidMount() {
  this.scrollToBottom();
}

componentDidUpdate() { // never triggers, even though it re-renders, when there's new data
  this.scrollToBottom(); 
}
....

Trying with reaction, doesn't really solve the issue, since it doesn't wait for the element to render and it will scroll to the second to last element instead.

componentDidMount() {
  reaction(
    () => someStore.someArray.length, // or prop, etc..
   (val, currentReaction) => {
     this.scrollToBottom();
     this.scrollbarReactionToDispose = currentReaction; 
     // not sure if I need to dispose reaction in componentWillUnmount
     // since it's in the same class and if it's the correct way to do so
   }
  );
}

componentWillUnmount() {
  this.scrollbarReactionDispose.dispose();
}

I've also tried going back to a React.Component, just to be sure, but no luck. I don't know if I'm missing something.

Thanks.

[winterbe]

I can confirm this issue. The method componentDidUpdate in a @observer class-based component no longer triggers. I have to go back to mobx-react 5 until this is fixed.

[jrmyio]

I thought i was going crazy ^_^ until I saw this issue.

Same here.

[danielkcz]

You shouldn't be using PureComponent if you are expecting cDU to be called. What PureComponent does it runs a shallow comparison of props in shouldComponentUpdate and since MobX store have a stable reference, it will be always the same. Unless you pass in some prop that has changed, it cannot work like that.

In other words, React lifecycle methods cannot be used together with MobX. If it was working for you in the previous version, it must have been some bug more likely.

Or provide a working example (with CodeSandbox) if you still think there some other problem.

[danielkcz]

I am sorry, I was too quick to jump to the conclusion here. I haven't used lifecycle methods for like 2 years now (praise the Hooks).

What V6 does differently is that it wraps the render method to the <Observer />. That means any observable change in render stays contained in that render and won't re-render the containing class component and as such no lifecycle methods will be called.

I am not entirely sure if it's intended, so I am going to call @mweststrate here for help.

---

Meanwhile consider migrating to Hooks :) If you dislike the first line, have a look at https://mobx-react.js.org/recipes-migration

const SomeList = inject('someStore')(observer(() => {
  React.useLayoutEffect(() => {
    // this will run after every render
    scrollToBottom()
  })

  // render your list
}

[winterbe]

In my case I’m not using PureComponent but Component. In some rare cases I need a way to execute arbitrary code after a component updates. That’s why I need componentDidUpdate.

I like functional components as well but I’m maintaining a large class-based component codebase which cannot be easily migrated to hooks over night. That’s why 100% class component compatibility in Mobx is so important for us (and I guess for many others as well). Also it would be hard to teach new team mates that cDU only works for normal React components but not for observer components.

I so much appreciate the hard work you guys are doing here and hope that you’ll find a way to solve this issue. Please let me know if you need more input.

Regards Benjamin

Daniel K. notifications@github.com schrieb am Sa. 8. Juni 2019 um 08:31:

I am sorry, I was too quick to jump to the conclusion here. I haven't used lifecycle methods for like 2 years now (praise the Hooks).

What V6 does differently is that it wraps the render method to the <Observer />. That means any observable change in render stays contained in that render and won't re-render the containing class component.

I am not entirely sure if it's intended, so I am going to call @mweststrate https://github.com/mweststrate here for help.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx-react/issues/692?email_source=notifications&email_token=AADWNKIGT6E5C3QTGVF5MOLPZNG5DA5CNFSM4HVH5BJ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXHO23I#issuecomment-500100461, or mute the thread https://github.com/notifications/unsubscribe-auth/AADWNKJQZBIJUFBNPS6TAETPZNG5DANCNFSM4HVH5BJQ .

[danielkcz]

I like functional components as well but I’m maintaining a large class-based component codebase which cannot be easily migrated to hooks over night.

Who is saying you should migrate the whole app? If you look at the recipe I linked, we do support partial migration just fine. A great strategy is whenever you are changing some class component, go ahead and refactor that one.

[winterbe]

Unfortunately due to this issue I need to scan the whole codebase for observers that use cDU (and probably cWRP as well) and migrate those components because it’s not backward compatible with mobx-react 5.

And I have to teach our whole dev team (15 people) about functional components and hooks and how Mobx now differs from vanilla React components.

It’s not that easy and has to be scheduled properly. I have to stick with mobx-react 5 till then unless you can solve the problem somehow which imo would be much better not only for me but for the whole Mobx community. Because right now you are no longer compatible with vanilla React and the React team has no plans in deprecating class components.

Regards Benjamin

Daniel K. notifications@github.com schrieb am Sa. 8. Juni 2019 um 08:58:

I like functional components as well but I’m maintaining a large class-based component codebase which cannot be easily migrated to hooks over night.

Who is saying you should migrate the whole app? If you look at the recipe I linked, we do support partial migration just fine. A great strategy is whenever you are changing some class component, go ahead and refactor that one.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx-react/issues/692?email_source=notifications&email_token=AADWNKLS6FJ5CSY7NXVF7K3PZNKCNA5CNFSM4HVH5BJ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXHPGMY#issuecomment-500101939, or mute the thread https://github.com/notifications/unsubscribe-auth/AADWNKOE4WLZHQQMAM2KSY3PZNKCNANCNFSM4HVH5BJQ .

[danielkcz]

cWRP is deprecated in React anyway, so you will do yourself (and your team) good deed if you refactor that over time and stay future relevant.

You are free to use mobx-react V5 if a majority of your code base is class components. It's actually recommended in README. You can use mobx-react-lite alongside for functional components with hooks.

Thinking about it, mobx-react was always about re-rendering based on observable changes in a render method. Calling lifecycle methods in the process might have been a side effect of the implementation leading to bad patterns. But once again, @mweststrate should probably confirm that.

---

As a workaround for the initial problem with scrolling, one approach could be to make a separate component that does scrolling and render it alongside that list. So anytime list is rerendered, cDU of that internal component gets called as well.

class Scroller extends React.Component {
  componentDidMount() {
    this.props.onScroll()
  }
  componentDidUpdate() {
    this.props.onScroll()
  }
  render {
    return null
  }
}

@inject('someStore')
@observer
class SomeList extends React.PureComponent {
  render() {
    return <React.Fragment>
      <div>{list}</div>
      <Scroller onScroll={this.scrollToBottom} />
    </React.Fragment>
  }
}

Ugly as hell I know, but it's actually more composable, reusable and closer to how Hooks are working (everything in render). That way your team can start to move toward that paradigm shift slowly and when the time comes, refactoring would be much easier.

[RosenTomov]

I've migrated the components that use ComponentDidUpdate() to use Hooks.

Thanks for the replies. 👍

[mweststrate]

Another (lesser than proposed by Daniel, so just for completeness sake) workaround would be to split the component two; an 'outer' observer based component that graps all the observables needed, and an child, non-observer component that renders the data and has the necessary life cycle hooks.

On Sun, Jun 9, 2019 at 2:46 PM RosenTomov notifications@github.com wrote:

I've migrated the components that use ComponentDidUpdate() to use Hooks.

Thanks for the replies. 👍

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx-react/issues/692?email_source=notifications&email_token=AAN4NBFKYE6WHE5YZ5WTPU3PZT3RXA5CNFSM4HVH5BJ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXIJNYA#issuecomment-500209376, or mute the thread https://github.com/notifications/unsubscribe-auth/AAN4NBAKOK3FOMQQJV5RIRLPZT3RXANCNFSM4HVH5BJQ .

[danielkcz]

@mweststrate So I take it you don't consider lifecycle hooks not reacting to re-renders as a bug? It should be probably mentioned somewhere since it's a breaking change from V5.

[winterbe]

If cDU is not fixable would it be possible to throw an error or log a warning if someone accidentally implements cDU on a observer class component? That would help a lot because it’s not obvious that cDU does no longer work with Mobx.

Daniel K. notifications@github.com schrieb am So. 9. Juni 2019 um 16:39:

@mweststrate https://github.com/mweststrate So I take it you don't consider lifecycle hooks not reacting to re-renders as a bug? It should be definitely mentioned somewhere since it's a breaking change from V5.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx-react/issues/692?email_source=notifications&email_token=AADWNKOEJIU6HCB4HGZD2QLPZUIYPA5CNFSM4HVH5BJ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXILJFQ#issuecomment-500216982, or mute the thread https://github.com/notifications/unsubscribe-auth/AADWNKKC2VEI5DYGJMIFSITPZUIYPANCNFSM4HVH5BJQ .

[danielkcz]

@winterbe Well, but cDU will still fire up if you change component state or pass new props. You could end up with a bunch of false positive warnings. We could probably throw one-time warning with explanation and ideally with a link to README explaining how to solve the problem. PR welcome :)

[mweststrate]

It is definitely an oversight and should be mentioned indeed. But I don't think the problem is big enough to justify a non trivial solution.

Op zo 9 jun. 2019 17:58 schreef Daniel K. notifications@github.com:

@winterbe https://github.com/winterbe Well, but cDU will still fire up if you change component state or pass new props. You could end up with a bunch of false positive warnings. We could probably throw one-time warning with explanation and ideally with a link to README explaining how to solve the problem. PR welcome :)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx-react/issues/692?email_source=notifications&email_token=AAN4NBHJLBM4IVYVRDUFHMLPZUSA5A5CNFSM4HVH5BJ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXINBMA#issuecomment-500224176, or mute the thread https://github.com/notifications/unsubscribe-auth/AAN4NBDLCKJAE4J7PXCHFOTPZUSA5ANCNFSM4HVH5BJQ .

[jrmyio]

If this is not fixed, and even if there is going to be a warning in the docs, many devs are going to waist hours trying to Google how it's technically possible that a console.log in render() is not followed by your console.log in cDU. It's only after some time they will realize it could be mobx-react.

I think the general idea of how and when component functions are called should not be broken by a third party library. I get the feeling that people who switch to hooks find this a minor issue because they will never run into this problem any more. But not everyone is switching to hooks + there is years of code that has been developed in class components for which 'upgrading' makes no sense.

I hope this gets another look before this issue is going to become stale for months.

[danielkcz]

@ConneXNL Once the linked PR is released, it will show a warning first time it spots observer component with cDU. Are you sure it's actually that common pattern? Perhaps only for overly complicated components that do too many things at once which is wrong by the SRP principle already.

As I said above, for class-based code bases it's perfectly normal to stick to V5. If somebody upgrades major version without carefully examining changelog then they have to live with consequences. Don't you agree? That's why we have major versions, to allow for breaking changes that make the overall experience better in most other cases.

[winterbe]

One time warning + bold mentioning in the docs/ changelog is fine for me. I also think cDU is kinda rare. Sometimes it’s necessary for complex repositioning of fixed containers but that shouldn’t be needed too often.

The future of React is functions and hooks however class components will still be a thing probably for a long time unless the React team deprecates them (if ever).

People need time to get used to it and to adapt in existing codebases. So please don’t rush too fast with abandoning existing features or people will move away to other solutions.

Daniel K. notifications@github.com schrieb am Mo. 10. Juni 2019 um 10:23:

@ConneXNL https://github.com/ConneXNL Once the linked PR is linked, it will show a warning first time it spots observer component with cDU. Are you sure it's actually that common pattern? Perhaps only for overly complicated components that do too many things at once.

As I said above, for class-based code bases it's perfectly normal to stick to V5. If somebody upgrades major version without carefully examining changelog then they have to live with consequences. Don't you agree? That's why we have major versions.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx-react/issues/692?email_source=notifications&email_token=AADWNKMLYO53XY4XM2CQ6XTPZYFQXA5CNFSM4HVH5BJ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXJH43A#issuecomment-500334188, or mute the thread https://github.com/notifications/unsubscribe-auth/AADWNKI4EGR2LKNTWTHDOW3PZYFQXANCNFSM4HVH5BJQ .

[sbellone]

Hello, On my side, I managed to fix the issue by simply inverting the declaration of @inject() and @observer():

@observer
@inject('someStore')
class SomeClass...

@FredyC does that make sense regarding what's changed in V6? Is it a good fix or is it better to stick to V5 if we want to keep class-based components?

[urugator]

Personally I would reconsider using <Observer> for observer impl. There is already a disadvantage of the extra component in the tree. A well tested solution already exists.

The usefulness of class support at it's current state is questionable. If we're forcing people to refactor, we could just not support classes at all. People will either refactor to hooks or they can use <Observer> in classes, without leading to wrong conclusions about lifecycle methods, props reactivity, purity etc... Also note that lifecycle method is likely a reason for the class existence, otherwise the user would use functional component (even without hook support).

[danielkcz]

If we're forcing people to refactor, we could just not support classes at all.

Not exactly forcing, V5 can be used just fine without any hassle. And as it was said already, it's a fairly rare case where things get broken.

If someone needs to support legacy code, they should stick to legacy libraries. Same applies to any other libraries. Any time you upgrade major version it's highly likely you will need to refactor something. And if someone wants to slowly start refactoring to hooks, they can still use mobx-react-lite alongside.

The V6 is not any groundbreaking version with the label "must have". It intended as a bridge toward a brighter future. People shouldn't really be upgrading out of whim just because it's cool to have the latest version. Any serious business must consider risks of that step.

So far we have seen a SINGLE example of an actual problem. And that case was already refactored to hooks so apparently it wasn't such a big deal. Until we can see it's a major problem for many people, we shouldn't do more than provide a warning and show a way how to solve it. Sometimes it's actually good giving people a motivation to refactor legacy code and slowly improve things.

There is already a disadvantage of the extra component in the tree.

This is true, that's a tradeoff, but once again, if someone is bothered by that, they can stick to V5 or refactor.

[danielkcz]

@sbellone Seems to me it's working by an accident, if that's really true :) Cannot imagine why it would change anything really.

[urugator]

V5 can be used just fine without any hassle.

It can't because it doesn't support hooks in functional components.

However my point is that V6 in it's current form would be actually better without class support, because it's hardly useful and it will generate confusion. It's not usable as transitioning bridge or how you wanna call it. BUT we have already decided to support classes in V6, therefore I think we should stick to that promise instead of coming with excuses. It's a bug, it wasn't anticipated or considered. Otherwise I would suggest deprecating class support in V6.

it's a fairly rare case

I could argue that cDU is the second most needed method after render. Again ... if you don't need cDU you likely don't need a class or you're not using class already. So in one of the few situations where class is actually needed, it's not supported.

If someone needs to support legacy code

Classes aren't legacy code. And again ... you can still use classes even without explicit class support, because of <Observer>.

So far we have seen a SINGLE example

Excluding us I see 6 different ppl already interested in this issue.

[danielkcz]

It can't because it doesn't support hooks in functional components.

Not true. You can still use mobx-react-lite alongside the same way it was before V6 got out. Effectively nothing is forcing you to use V6 for real.

Otherwise I would suggest deprecating class support in V6.

That's quite an excessive don't you think? Do you see masses of people complaining that it's broken? I don't. You are trying to solve something that doesn't ask for solving. Do you personally have a component that suffers from this problem? I don't see anybody else to be really complaining they need the old behavior. On the contrary, they are ok with warning and refactoring.

I could argue that cDU is the second most needed method after render.

Sure, in non-observable components you want to do something when props or state update. It's not called componentDidRender you know? If some observable has caused re-render, it's arguably not an update. Even if shouldComponentUpdate would trigger somehow, it would return false because props/state hasn't changed. So current behavior is more correct semantically than V5 was.

[urugator]

It's not called componentDidRender

Render is part of the component update. cDU guarantees that rendered output has been flushed to DOM. Btw getDerivedStateFromProps and getSnapshotBeforeUpdate are affected as well.

That's quite an excessive don't you think?

• half of the lifecycle methods doesn't work
• if V6 is really inteded as a bridge, it translates to not supporting classes in the future
• every workaround requires refactoring or using older version
• you're discouraging the use of classes literally everywhere
• classes are still usable with <Observer>

[danielkcz]

Once again, do you actually have more examples that show the problem? All I see here is one practical code that used that "feature" and is easily fixable. Let's not debate about rights and wrongs here, that could be endless.

I am fairly convinced that most of the classes with observer don't need to run any update logic based on observable change. They will still update when props change, we haven't broken that.

If you want to really fight for this, feel free to set up a PR that would revert this breaking change while keeping other improvements. Maybe you will realize it's not that easy as it looks.

[jrmyio]

@fredyc I think what @urugator is trying to say you are better of saying classes aren't supported than saying they are supported while XYZ doesn't work. Otherwise it's going to be very confusing for people, especially since they have been fully supported in <= 5.

One can better include two libraries, one for non-hook classes and one for hook classes (if both can't be put in one package). I am just not sure that sticking one of the packages to an "older version" is the way to go as what would be the difference between mobx-react and mobx-react-lite? What will we miss out on the future when sticking with mobx-react@5?

Bottom line, we need at least some way to support both hook and class components to live in the same code-base.

[danielkcz]

classes aren't supported than saying they are supported while XYZ doesn't work

And I am trying to explain that XYZ wasn't actually supposed to work in V5 and was a side effect of the implementation. If people abused that bug, now is a good time to rethink that approach. Nobody is forced to upgrade overnight, though.

Once again, I would like to see more real examples. I am still convinced it's not affecting that much cases to be an actual trouble. Please, show me other use cases where you need to cDU to fire because of observable change AND it's hard to fix it by some small refactoring. Otherwise, we are just walking in circles and trying to correct a mistake that's not such a problem in the end.

what would be the difference between mobx-react and mobx-react-lite? What will we miss out on the future when sticking with mobx-react@5?

V6 is just repackaged mobx-react-lite that still supports class components and it replaces legacy context for new Context API. V6 has no functional changes that wouldn't be already present in V5. Having mobx-react-lite alongside was a viable approach for many months now and there is no reason why it would be any different now.

[winterbe]

Just some stats from my project: From a total of 750 React components, 34 components implement componentDidUpdate with 15 components being observers with cDU. It's definitely an edge case but cDU is part of the public React API and it's kinda weird that Mobx is now only partially compatible with React with half of the lifecycle methods no longer working.

Also sticking with v5 is not an option in the long term because I guess nobody is maintaining this branch currently in case of new bugs? Also React 16.8 was released just 4 months ago. People need time to adapt. Not everyone is starting new small projects every couple of months. I'm ok with abandoning out-dated features from time to time, but not at that pace. The React team has clearly stated that they don't abandon classes any time soon (because of dog fooding).

[mweststrate]

Some observations, things we can in general probably all agree upon:

1. The breakage wasn't intended, but an oversight, apologies!
2. Upgrading shouldn't lead to surprises
3. Classes are still supported (leaving in the middle what that precisely means). If you are not interested in classes, I'd recommend mobx-react-lite anyway. Whether classes are old fashioned or not is irrelevant and to be decided by the community at large. Fact is: many code bases do still have classes. We are not the fashion police on how people should use React. For the same reason, mobx-react shouldn't be written as if classes are being a temporarily thing. Existing projects will be maintained for years, and will want to benefit from new React features. MobX-react shouldn't block users from updating React
4. mobx-react should make transitioning to hooks gradually easy. And try to avoid introducing confusion such as being able to import observer from two different packages.
5. If something worked in a previous version, that shouldn't be declared as a bug in hindsight :)

So I think atm there are currently two solutions:

1. Restore the class based implementation of observer
   • benefit: same behavior as in V5 for classes
   • benefit: no more wrapper components
   • con: duplicated logic
   • con: no way to leverage the future hooks-based devtools integration
2. Keep things as is, fix documentation and changelog
   • Life cycle hooks were always quite inconsistent in mobx-react anyway (e.g. componentWillReceiveProps did not receive two sets of props), but rarely anybody complained about that, as in the MobX model life cycle hooks are much less relevant anyway
   • componentDidUpdate is actually quite useful, but not too hard to work around probably in most cases: either use <Observer>, or split the component into two, with a "parent" observer grabbing all the observables, and "child" classic class component. This should be very clearly documented.
   • This approach is only feasible if this is only a rare case, that is, if an entire code base requires less than 5 components being adjusted, I could live with it.

Personally speaking, at this moment (without more info / complaints) I could perfectly live with either solution; restore the old observer or document the new limitations very clearly.

[danielkcz]

@winterbe

Just some stats from my project: From a total of 750 React components, 34 components implement componentDidUpdate with 15 components being observers with cDU.

Thanks for the stats. Would be interesting to know, how many from those 15 components actually need to act based observables change within cDU. Implementing cDU doesn't effectively means such a component is broken if it cares about props change only. It depends on what cDU actually does. If it doesn't need the latest DOM state, it will work just fine. I will expand documentation PRs bit more on this.

@mweststrate Thanks for a nice summarization.

So I think at this point we should probably wait if more people have an actual problem with that change. The original author of this issue was fine with refactoring to hooks. GIven that V6 actually requires React 16.8, it means that people have Hooks at their disposal and can attempt at refactoring if it's not against some of their policy. But then again why would they upgrade to V6 if they won't use hooks ever?

[winterbe]

@FredyC In all cases I've seen componentDidUpdate is expected to run after every re-render of the component to perform manual DOM updates such as positioning of containers, handling scroll positions, calling native APIs etc. or firing some legacy events (which still have to be be proper migrated to observable state). So it's crucial that cDU fires after the observer renders on observable updates.

[danielkcz]

@winterbe Ok, so you are saying it's nearly impossible for your team to refactor those components? Either to hooks or the workaround, I will repeat just for the sake of everyone.

function Lifecycle({ onRender }) {
  // runs after every render
  React.useEffect(onRender)
  return null
}

@observer
class SomeList extends React.PureComponent {
  componentDidUpdate() {
  }
  render() {
    return <React.Fragment>
      <div>{list}</div>
      <Lifecycle onRender={this.componentDidUpdate} />
    </React.Fragment>
  }
}

Edit: I am thinking if we could actually include such component to the output automatically whenever we detect cDU used. That way we could keep current implementation and satisfy these use cases.

[winterbe]

No it’s not impossible. We have a set budget for refactorings every month. But it has to be coordinated within our team of 15, (many of them doing mainly backend development) and other refactoring topics which are already scheduled.

But the workaround you mentioned looks kinda interesting for an intermediate solution until we finally jump on the hooks train. Will give it try soon if it’s sufficient for us.

Daniel K. notifications@github.com schrieb am Di. 11. Juni 2019 um 12:21:

@winterbe https://github.com/winterbe Ok, so you are saying it's nearly impossible for your team to refactor those components? Either to hooks or the workaround, I will repeat just for the sake of everyone.

function Lifecycle({ onRender }) { // runs after every render React.useEffect(onRender) return null }

@observerclass SomeList extends React.PureComponent { componentDidUpdate() { } render() { return

{list}

  <Lifecycle onRender={this.componentDidUpdate} />
</React.Fragment>

} }

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx-react/issues/692?email_source=notifications&email_token=AADWNKOP34CWCMVH3I42JHDPZ54BTA5CNFSM4HVH5BJ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXMU7RI#issuecomment-500780997, or mute the thread https://github.com/notifications/unsubscribe-auth/AADWNKMJERDPMINFSHJ6C5TPZ54BTANCNFSM4HVH5BJQ .

[urugator]

Even though the mentioned workaround may work most of the time it's potentially dangerous. You can't simply leak parent's state into a child via callback. It would have to be done like this:

function Lifecycle({ onUpdate, deps }) {
  // Layout effect to mimick cDU   
  React.useLayoutEffect(() => {
    onUpdate(...deps)
  }, deps)  
}

// intentionally outside to avoid access to "this"
function mySideEffect(a,b,c) {
  // whatever
}

@observer
class SomeList extends React.PureComponent {  
  render() {
    return <React.Fragment>
      <div>{list}</div>
      <Lifecycle onUpdate={mySideEffect} deps={[this.state.a, this.props.b, this.prop.observable.c]}  />
    </React.Fragment>
  }
}

[danielkcz]

@urugator Wait, what? React will fire cDU on props/state change on its own. Why are you still talking like we have broken React? 😆 You don't need those deps there unless I am missing some use case here, but that's not the purpose of workaround.

Nevermind, I get it now. Forgot that cDU has previous props/state in its arguments. That makes it trickier, but still not impossible.

[danielkcz]

Just for the reference, looks like using Observer has other unforeseen consequence ... #699

That kinda moves the weight on the side of reverting to the "old way" even for a cost of duplicated and less optimized code.

[urugator]

duplicated and less optimized code

What do you mean by "less optimized"?

[winterbe]

Nevermind, I get it now. Forgot that cDU has previous props/state in its arguments. That makes it trickier, but still not impossible.

However your solution should work well in case the arguments of cDU are not used which is often the case in our project. IMO in most Mobx cases when using cDU on an observer you probably just want to run some code on every re-render regardless of previous props/ state because you usually react to observable changes (which is not passed to cDU) instead of setState.

[urugator]

I am sorry, I take it back, It should be safe. I was worried about something else than prevState/prevProps. Just use useLayoutEffect if you want to mimick cDU (use it for DOM manipulation).

[mweststrate]

I think we are slowly uncovering to many edge cases. All "fixable", but not intuitive enough I think. With some remorse I propose to include the classic implementation for classes: #703. (It kinda make sense to keep an old implementation for an old concept around probably, but I do dislike the duplication with mobx-react-lite now).

[iMoses]

Please make this a priority. Not everyone is moving to hooks as class components are still wildly supported and maintained, and this won't change anytime soon. Also, many code bases use a combination of both function and class components, depending on the complexity and use case of the component. Please don't become opinionated on this matter and allow for both options to work interchangeably.

[pavelserbajlo]

Had to downgrade to 5.4.3 due to this. Forcing developers to address their (not-that-old!) technical debt outside of their timelines and plans is hard to justify. If this is the mindset of maintainers of this repo, we'd rather invest our time into finding our way around this repo to prevent similar situation in the future.

[danielkcz]

@pavelserbajlo Nobody has forced you to upgrade to V6 in the first place. I said it here already. Every move to next major version should be carefully considered, not to be done out of boredom or something. Just saying that it's not the brightest idea to be upgrading when running on a strict timeline.

On the other hand, without these first explorers, the bugs would not reveal themselves 😆

[pavelserbajlo]

@FredyC that's a fair point. I'm having hard time finding any word of caution in README or anywhere else, though. I'd suggest to add this somewhere to save some developer time worldwide :)

btw: we were not updating out of boredom, but rather because we're seeing mobx-react to randomly stop triggering observers on changes in actions, dozens of minutes into the running session and we wanted to check if it's something that was addressed already by any chance.

[danielkcz]

we're seeing mobx-react to randomly stop triggering observers on changes in actions. Dozens of minutes into the running session and we wanted to check if it's something that was addressed already by any chance

The best course of action is to reproduce that and create an issue. Bugs won't fix "by any chance" if we don't know about them ;) But chances are it's more about the wrong way of using something as MobX is pretty much predictable.

I'm having hard time finding any word of caution in README or anywhere else, though. I'd suggest to add this somewhere to save some developer time worldwide :)

You are right, there wasn't any warning or anything, but it was mainly because we were convinced that V6 will work just fine. There just turned out to be more variables and concerns that we were not aware of and had no test cases for them.

[pavelserbajlo]

@FredyC we spent hours debugging this and we are clueless at this point. It stops working randomly, without any warnings or errors and we already confirmed that actions are being called and observables are being updated. Our observers are not triggered, though. I don't think it's relevant to this thread and I might be starting a new issue here if we have more information on this.

[danielkcz]

Btw, V6 was in beta for roughly 4 months and nobody reported any of those issues. We could easily prevent this awkwardness that if people were less reluctant to spend some time with these versions. It applies especially to code bases that have good test coverage and can discover bugs just by using beta without endangering production.

Perhaps the existence of beta wasn't public enough? It could be interesting to run some poll to see how many people actually noticed that beta version 🤔

[pavelserbajlo]

@FredyC I haven't noticed for instance, upgrading the version over npm took me straight to V6.

[danielkcz]

I haven't noticed for instance, upgrading the version over npm took me straight to V6.

Yea, that's kinda the problem of NPM. It could be really useful if we were able to notify about the existence of other release channels while the latest is being installed. I have seen deprecation notices but nothing else. Anybody heard about something like that?

[mweststrate]

Fixed in 6.1.0