billmalarky/react-native-image-cache-hoc

Warning on setState

Closed this issue · 16 comments

khrizt commented

Hey @billmalarky

Sorry for being so verbose, but I found that in the line

react-native-image-cache-hoc/lib/imageCacheHoc.js

Line 153 in 5bca7d2

localFilePath => this.setState({ localFilePath }),
of imageCacheHoc file there is a setState without checking if the component is mounted. Maybe keeping a flag and setting to true in componentDidMount and false in componentDidUnmount would be a solution.

It happens of images that I show and hide real fast, I'm using it inside a chat app so this can happen. I can do a PR if you think it's a good idea.

billmalarky commented
khrizt commented

It's strange because I saw a warning with the message: "Can only update a mounted or mounting component. This usually means you called setState, replaceState, or forceUpdate on an unmounted component. This is a no-op"

And the stack trace pointed to that line... 😕

I'll take a deeper look, but if you think of something, let me know. Thanks!

billmalarky commented
khrizt commented

Yes, I'm on 1.3.1. But I'll do some more checkings.

khrizt commented

Ok, finally I did more checkings. I've put a flag in the component that is set to true in componentDidMount and false in componentWillUnmount and a console.log in that line. Turns out that the line is called when the flag is false. I paste some code because my explanation sucks.

    constructor(props) {
      super(props);

      // [...]

      this.mounted = true; // <---- FLAG
    }

    // [...]

    async componentDidMount() {
      // [...]

      this.cancelLocalFilePathRequest = makeCancelable(
        this.fileSystem.getLocalFilePathFromUrl(
          traverse(this.props).get(['source', 'uri']),
          permanent,
        ),
        localFilePath => {
          console.log('Im here and the flag is: ' + this.mounted);
          this.setState({ localFilePath });
        },
        error => console.error(error), // eslint-disable-line no-console
      );
      this.mounted = true; // <--- SET flag to true
    }

    async componentWillUnmount() {
      // [...]
      this.mounted = false; // <--- SET flag to false
    }

And I get one console log with "I'm here and the flag is false". I've seen that the method cancelLocalFilePathRequest is called on unmounting, but as makeCancelable returns a Promise, is it possible that while the promise is being resolved some calls go through?

Sorry for the huge text 😄

billmalarky commented

The entire point of the makeCancelable() package is to prevent the "onSuccess" method from being fired if the initial this.fileSystem.getLocalFilePathFromUrl() promise takes too long to resolve and componentWillIUnmount() is fired first.

So if componentWillUnmount() is being fired before this.fileSystem.getLocalFilePathFromUrl() resolves, and STILL the "success" handler is getting fired (the part that calls setState()), that suggests to me there is a bug in the makeCancelable package.

I will have to investigate further but probably won't be able to for a few days. Shouldn't be a big deal since this is related to warnings that won't block your dev.

I'm going to try to get your PureComponent ticket handled today or tomorrow.

khrizt commented

It's not a problem, just an annoying warning. If you want I can do some investigation and report if you point me in the right direction.

billmalarky commented

It's annoying though because I implemented the makeCancelable library specifically to resolve this issue.

Is there some way you can share code with me that would allow me to recreate it?

That is, spin up a throwaway app with react-native init, install react-native-image-cache-hoc and then quickly recreate the issue you are seeing in your proprietary app and send me the code? I'm struggling to recreate this issue.

Thanks and Merry Christmas soon :-)

khrizt commented

Ok, I'll try to do it by tomorrow.

Merry Christmas!! 🎄

khrizt commented

I'm trying to reproduce the behaviour in a simpler app with no luck yet. I'll keep you posted.

khrizt commented

Ok, finally! Here's the repo with the warning appearing: https://github.com/khrizt/image-cache-hoc-example Please don't look too much at the code 😋, I started adding things until I could reproduce it. If you need some clarification, please let me know.

billmalarky commented

Haha I assure you I won't judge, it would be weird if it was too good lol.

Greatly appreciate this. I Probably won't have time to fix until after Christmas but it's near the top of my todo list.

billmalarky commented

Okay I figured out the issue thanks to your test case.

The edge case occurs when componentDidMount() fires, but then execution of the function is paused while awaiting this.fileSystem.getFileNameFromUrl() to complete here.

Since the componentDidMount() function does not resume execution until the promise returned by getFileNameFromUrl() resolves, if the component is quickly unmounted after being mounted, componentWillUnmount() will fire before componentDidMount() resumes. In this scenario we end up getting to this code before we expect to, and thus not cancelling the makeCancelable() onfulfilled handler.

I'm going to rip out the makeCancelable library and just implement this logic myself.

NOTE: Patching this with _isMounted property as suggested here.

An easy migration strategy for anyone upgrading their code to avoid isMounted() is to track the mounted status yourself. Just set a _isMounted property to true in componentDidMount and set it to false in componentWillUnmount, and use this variable to check your component’s status.

billmalarky commented

Resolved #9

khrizt commented

Awesome man!!!! Thanks a lot! And Happy New Year!! 🎆

billmalarky commented