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

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 <#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 <#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 ***@***.***> 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 <#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 <#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 <#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 <#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?

[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 <React.Fragment> <div>{list}</div> <Lifecycle onRender={this.componentDidUpdate} /> </React.Fragment> } } — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#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

[mweststrate]

@pavelserbajlo make sure HMR is disabled and that there is only one instance of mobx and mobx-react in your module tree. I've seen both HMR, and improperly organized dependencies as primary usecases for random weird behavior in the past. (if that doesn't help, please take the conversation to a separate issue as indicated by @FredyC, this one is closed now)

[winterbe]

Fixed in 6.1.0

Just wanted to say Thank You for the pragmatic fix. After I had to go back to v4 due to this issue, I've bumped the version back to latest 6.1.0 and everything seems to work like expected. Now my team can adopt hooks iteratively without a hurry or any further migrations. 👏

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs or questions.