FormSpy: re-mounting subscription memory leak from render-phase side-effects

[Sharakai]

Are you submitting a bug report or a feature request?

Bug report

What is the current behavior?

The FormSpy constructor has a side-effect which, when using React.StrictMode or React async mode, causes a subscription memory leak.

React.StrictMode helps detect unexpected side effects within the render phase by invocating some (most) of the React lifecycle methods twice - this includes the constructor. (See here for details/clarification).
The FormSpy constructor runs the following (here):

this.subscribe(props, (state: FormState) => { ...

which in turn runs (here):

subscribe = (
    { subscription }: Props,
    listener: (state: FormState) => void
  ) => {
    this.unsubscribe = this.props.reactFinalForm.subscribe(
      listener,
      subscription || all
    )
  }

hence setting the this.unsubscribe class-member variable. Because the constructor is ran twice, the first instantiation's unsubscribe value is overwritten, and only the 2nd call's is ever executed in componentWillUnmount (here).

According to React's docs on async rendering, render phase lifecycles may be invoked more than once, and so should can not have side-effects.

Because the above methods might be called more than once, it’s important that they do not contain side-effects. Ignoring this rule can lead to a variety of problems, including memory leaks and invalid application state.

What is the expected behavior?

All render-phase lifecycle methods should be side-effect free, allowing for compatibility with React.StrictMode and React async rendering mode.

Sandbox Link

https://codesandbox.io/s/n1p24n90xj
It's a little difficult to "show" the issue, but this will do it.

1. Open & clear the Console
2. Add 1 letter to the end of either field (e.g. John -> Johna
3. Check the console and you'll see "Values changed" twice - This is because the constructor was invocated twice, leading to 2 subscriptions.
4. Click the "Re-mount FormSpy" button & clear the console
5. Add 1 letter to the end of either field (e.g. Doe -> Does)
6. Check the console and you'll see "Values changed" three times - The FormSpy has re-mounted, hence invocated componentWillUnmount (commit phase) once and the constructor (render phase) twice. One of the original mounting's subscriptions has been retained and not been unsubscribed from during unmounting.

The more you re-mount the FormSpy, the more subscriptions are left over. Mount it 10 times (so the button says "Mounted 10 times") and you'll see "Values changed" 11 times on every field change (9 stale subscriptions which should have been removed, and 2 new subscriptions).

What's your environment?

• Final Form v4.11.0
• React-Final-Form v4.0.2
• React v16.5.2 (or any version with React.StrictMode, or any version with async rendering enabled)

Other information

Although the codesandbox example is non-real-world, we hit this problem in a real-world scenario where our form expands revealing extra form Fields and FormSpys.

I am more than happy to work on this and PR it, but will obviously needs plenty of review. It will also be a breaking change, as the subscription (and subsequent onChange being called) will be moved from the constructor to componentDidMount

[Sharakai]

Note: this work will also make it React 17+ compliant.

I'm also trying to ensure all tests pass as they currently do, and only change / fix what absolutely requires it.
I'll PR from my fork within the week for discussion.

EDIT: Didn't realise I had so little free time. Still working on this, and hope to get it PR'd up soon.

[Sharakai]

@erikras (re #427 (comment))
I cannot reply to the PR, as it's locked to contributors and members.

Apologies if you took my comment on the PR as an offence/attack - I certainly didn't mean to.
I closed the PR as I was getting emails regarding merge conflicts on every other PR that was merged in, and I was going to re-open after I got time to raise a secondary, contrasting PR.

Thanks for raising and taking a look, regardless. I'll try to get some time to pick it up again, rebase, etc.
I'll also be raising a few issues and making a few PRs for react-final-form-hooks, as we're using extensively on my current project. (excellent job with final-form, by the way.)

[callmeberzerker]

In the mean time...

if (process.env.NODE_ENV !== 'production') {
  const pattern = /is deprecated and will be removed in the next major version/;
  const { warn } = console;
  console.warn = function warnWithoutReactViolations(message, ...rest) {
    if (!pattern.test(message)) {
      warn(message, ...rest);
    }
  };
}

[erikras]

This should be fixed in v5. Reopen if not.

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

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