Add test for proper reaction disposal in StrictMode

[RoystonS]

This is a PR for a test to illustrate the issue with Concurrent Mode (and, more problematic, in StrictMode), as requested in #53 (comment)

(It currently fails, so you may not want to merge this right now...)

[danielkcz]

I think I would rather prefer if you would be testing low-level useObserver since this adds few extras (like memo) in the mix. Would you be so kind to change it, please?

[danielkcz]

Please use jest-console instead of this boilerplate :) It's even used in this test file already :)

[RoystonS]

Yup. Will do. I don't normally use Jest, so I wasn't aware of that one.

[RoystonS]

I think I would rather prefer if you would be testing low-level useObserver since this adds few extras (like memo) in the mix. Would you be so kind to change it, please?

I thought by putting my code inside the runTestSuite where it is, that the test would be run against both observer and useObserver?

[danielkcz]

I thought by putting my code inside the runTestSuite where it is, that the test would be run against both observer and useObserver?

Well, it's true, but you don't need to test observer really because it's a tiny wrapper around the useObserver. The "bug" we are testing against is within the useObserver so I vote for testing as close as possible to the actual problem occurrence, not the abstraction.

[RoystonS]

Ok. I did originally start to create a separate useObserver.test.tsx file (as there wasn't one) but the realisation that nobody had done that before, and that there were combined tests for both useObserver and observer made me change it in order to follow the existing pattern instead.

I'll pull it out into the separate useObserver.test.tsx test file.

(I'm also playing with a fix; a small change to useObserver allows me to track whether the component has been committed or not before invoking forceUpdate, which fixes my strict mode issue; disposing stray reactions even if their observables haven't changed isn't very hard, but a little tricky to test depending on the schedulding implementation.)

[RoystonS]

I've pulled it out into a separate file, and added a second test to verify not just that no console errors were emitted, but that the store is definitely unobserved after unmount.

[danielkcz]

Awesome! Looks great. Now to figure a solution ... 🤔

It's most likely silly, but wouldn't something like this work? Can you please try it against tests?

    const [isMounted, setMounted] = React.useState(false)
    const reaction = useRef<Reaction | null>(null)
    if (!reaction.current) {
        reaction.current = new Reaction(`observer(${baseComponentName})`, () => {
            if (isMounted) {
              forceUpdate()
            }
        })
    }

    React.useEffect(() => {
      setMounted(true)
    }, [])

There is of course problem of lingering Reaction, but @mweststrate already mentioned some way of clearing those up globally, so that's perhaps not an issue to solve here.

[RoystonS]

That'll probably work, but the setMounted call will trigger a second full render + commit for all observable components.

Using a ref achieves the same thing without the extra full round-trip:

    const committed = useRef(false)

    if (!reaction.current) {
        // First render for this component. Not yet committed.
        reaction.current = new Reaction(`observer(${baseComponentName})`, () => {
            if (committed.current) {
                forceUpdate()
            }
        })
    }

...
 
   // (Replaces the previous useUnmount call)
    useEffect(() => {
        committed.current = true
        return () => reaction.current!.dispose()
    }, [])

This does fix my StrictMode test. I'm just testing out some cleanup code that would dispose the stray reactions.

[RoystonS]

I've removed my second (leaked observable) test for now: I'll move it into a separate PR as the fix for that is more complicated. I've also added a fix for the StrictMode warning issue, so it might now be possible to consider merging this.

[coveralls]

Coverage decreased (-0.005%) to 99.301% when pulling d9f8fe5 on RoystonS:master into ed1634d on mobxjs:master.

[RoystonS]

(Want me to remove useUnmount as I seem to have remove the last use of it...?)

[danielkcz]

Yes please, ditch useUnmount and it's indeed ready for merge. Thanks!

[RoystonS]

useUnmount is no more.

[RoystonS]

I've been testing out this change in our codebase (with 100+ observer components) and it caused a breakage in one particular scenario linked to changes that occurred between render and first commit.

I'd already added an extra unit test in #121 to identify an existing issue where changes could be lost, and that test failed both before and after the #119 change. However, running the same test in non-strict mode produced a different behaviour: before #119 it passed, but after my #119 change, it fails, missing the change.

I see two options here:

1. I could push here the extra, failing unit test (which I've already pushed into Schedule uncommitted reaction cleanup #121), and a fix I have (which involves another ref, checking if a change occurred between the render and commit, and forcing an update). That fixes the new unit test, and fixed the issue in our codebase, and stops changes during component setup being lost.
2. We could back out the Add test for proper reaction disposal in StrictMode #119 change completely. That would leave strict mode noisy and with some changes during component setup lost.