microsoft/tslint-microsoft-contrib

Suggest rule: pair-react-dom-render-unmount

lijunle opened this issue · 4 comments

lijunle commented

Rule Suggestion

Is your rule for a general problem or is it specific to your development style?

Yes

What does your suggested rule do?

The team did some investigation on performance and found there is memory leak because:

  1. We call ReactDOM.render without ReactDOM.unmountComponentAtNode to unmount it.
  2. We capture the returning value from ReactDOM.render without set it back to undefined when disposed.

The idea of the rule is to do two things:

  1. In one file, the number of calls to ReactDOM.render must equal to the number of calls to ReactDOM.unmountComponentAtNode. Technically, they don't need to be in the same file, but it is easier to maintain/review if they appear as a pair.
  2. If there is a variable capturing the returning value from ReactDOM.render method call, that variable must be set to undefined or null something in the same file.

List several examples where your rule could be used

SharePoint web parts.

JoshuaKGoldberg commented

Hi @lijunle, thanks for posting the results of your investigation here!

I'm not convinced this rule is something that should be promoted as a general-use good practice: there are times when you wouldn't want to call unmountComponentAtNode. For example, some React applications only ever call ReactDOM.render once on page startup. It'd be weird for them to start getting a TSLint complaint that they need to do more with the results of that.

Suggestion: how about publishing this as a standalone npm package that contains the rule and a README.md explanation of when & why it's useful?

lijunle commented

@JoshuaKGoldberg I think that is a good point to not enable it by default. But even though a big application call ReactDOM.render once when the page startup, it could better to handle ReactDOM.unmountComponentAtNode on page close. That is a good practice.

JoshuaKGoldberg commented

ReactDOM.unmountComponentAtNode on page close. That is a good practice.

Really? Intuitively, if the page is closing, wouldn't that add unnecessary work that makes closing the page slower? Can you explain why or point to references that support your proposal? I might have to file some bugs if what you're saying's accurate 😄.

At any rate, this repo doesn't take in rules constrained to a single library. You'll want to file this issue on tslint-react.

lijunle commented

Fine. I open an issue in tslint-react: palantir/tslint-react#195

  • 👍1