Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

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

Closed
lijunle opened this issue Dec 30, 2018 · 4 comments
Closed

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

lijunle opened this issue Dec 30, 2018 · 4 comments
Labels
External Issues that should be addressed in other repositories. Status: In Discussion Please continue discussing the proposed change before sending a pull request.
Milestone

Comments

@lijunle
Copy link
Member

lijunle commented Dec 30, 2018

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 JoshuaKGoldberg added Status: In Discussion Please continue discussing the proposed change before sending a pull request. Type: Rule Suggestion Adding a new rule that doesn't yet exist here or in TSLint core. labels Dec 31, 2018
@JoshuaKGoldberg
Copy link

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
Copy link
Member Author

lijunle commented Jan 2, 2019

@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
Copy link

JoshuaKGoldberg commented Jan 2, 2019

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.

@JoshuaKGoldberg JoshuaKGoldberg added External Issues that should be addressed in other repositories. and removed Type: Rule Suggestion Adding a new rule that doesn't yet exist here or in TSLint core. labels Jan 2, 2019
@lijunle
Copy link
Member Author

lijunle commented Jan 3, 2019

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

@IllusionMH IllusionMH added this to the 6.1.0-beta milestone Feb 19, 2019
@IllusionMH IllusionMH modified the milestones: 6.1.0-beta, None Mar 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
External Issues that should be addressed in other repositories. Status: In Discussion Please continue discussing the proposed change before sending a pull request.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants