Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Always trigger componentWillUnmount in StrictMode #26842

Merged
merged 1 commit into from
Jun 1, 2023
Merged

Always trigger componentWillUnmount in StrictMode #26842

merged 1 commit into from
Jun 1, 2023

Conversation

tyao1
Copy link
Contributor

@tyao1 tyao1 commented May 24, 2023

Summary

In StrictMode, React currently only triggers componentWillUnmount if componentDidMount is defined. This would miss detecting issues like initializing resources in constructor or componentWillMount, for example:

class Component {
  constructor() {
     this._subscriptions = new Subscriptions();
  }
  componentWillUnmount() {
     this._subscriptions.reset();
  } 
}

The PR makes componentWillUnmount always run in StrictMode.

How did you test this change?

yarn test ci

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels May 24, 2023
sammy-SC
sammy-SC approved these changes Jun 1, 2023
View reviewed changes
packages/react-reconciler/src/ReactFiberCommitWork.js
instance.componentDidMount();
} catch (error) {
captureCommitPhaseError(fiber, fiber.return, error);
if (typeof instance.componentDidMount === 'function') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR always sets MountLayoutDev on mount regardless if instance.componentDidMount exists, and when the flag is set, this code path always runs.

@tyao1 tyao1 merged commit 8110222 into main Jun 1, 2023
github-actions bot pushed a commit that referenced this pull request Jun 1, 2023
@tyao1
Always trigger componentWillUnmount in StrictMode (#26842)
d6f9de5 
<!--
  Thanks for submitting a pull request!
We appreciate you spending the time to work on these changes. Please
provide enough information so that others can review your pull request.
The three fields below are mandatory.

Before submitting a pull request, please make sure the following is
done:

1. Fork [the repository](https://github.com/facebook/react) and create
your branch from `main`.
  2. Run `yarn` in the repository root.
3. If you've fixed a bug or added code that should be tested, add tests!
4. Ensure the test suite passes (`yarn test`). Tip: `yarn test --watch
TestName` is helpful in development.
5. Run `yarn test --prod` to test in the production environment. It
supports the same options as `yarn test`.
6. If you need a debugger, run `yarn test --debug --watch TestName`,
open `chrome://inspect`, and press "Inspect".
7. Format your code with
[prettier](https://github.com/prettier/prettier) (`yarn prettier`).
8. Make sure your code lints (`yarn lint`). Tip: `yarn linc` to only
check changed files.
  9. Run the [Flow](https://flowtype.org/) type checks (`yarn flow`).
  10. If you haven't already, complete the CLA.

Learn more about contributing:
https://reactjs.org/docs/how-to-contribute.html
-->

## Summary

<!--
Explain the **motivation** for making this change. What existing problem
does the pull request solve?
-->
In StrictMode, React currently only triggers `componentWillUnmount` if
`componentDidMount` is defined. This would miss detecting issues like
initializing resources in constructor or componentWillMount, for
example:
```
class Component {
  constructor() {
     this._subscriptions = new Subscriptions();
  }
  componentWillUnmount() {
     this._subscriptions.reset();
  }
}
```

The PR makes `componentWillUnmount` always run in StrictMode.

## How did you test this change?

<!--
Demonstrate the code is solid. Example: The exact commands you ran and
their output, screenshots / videos if the pull request changes the user
interface.
How exactly did you verify that your PR solves the issue you wanted to
solve?
  If you leave this empty, your PR will very likely be closed.
-->
`yarn test ci`

DiffTrain build for [8110222](8110222)
@kassens kassens deleted the ty-strict-mode-cwunmount branch June 1, 2023 18:47
@styfle styfle mentioned this pull request Jun 30, 2023
Merged
timneutkens pushed a commit to vercel/next.js that referenced this pull request Jul 4, 2023
@styfle @shuding
chore(deps): bump react 18.3.0-canary-1fdacbefd-20230630 (#52005)
9e4b87d 
This PR updates the vendored react dependencies using `pnpm sync-react`

### React upstream changes

- facebook/react#27028
- facebook/react#27027
- facebook/react#27019
- facebook/react#26954
- facebook/react#26987
- facebook/react#26985
- facebook/react#26933
- facebook/react#26625
- facebook/react#27011
- facebook/react#27008
- facebook/react#26997
- facebook/react#26989
- facebook/react#26955
- facebook/react#26963
- facebook/react#26983
- facebook/react#26914
- facebook/react#26951
- facebook/react#26977
- facebook/react#26958
- facebook/react#26940
- facebook/react#26939
- facebook/react#26887
- facebook/react#26947
- facebook/react#26945
- facebook/react#26942
- facebook/react#26938
- facebook/react#26844
- facebook/react#25510
- facebook/react#26932
- facebook/react#26896
- facebook/react#26913
- facebook/react#26888
- facebook/react#26827
- facebook/react#26889
- facebook/react#26877
- facebook/react#26873
- facebook/react#26880
- facebook/react#26842
- facebook/react#26858
- facebook/react#26754
- facebook/react#26753
- facebook/react#26881

### Related

Closes #49409 (by facebook/react#26977)
fix NEXT-1189

Co-authored-by: Shu Ding <[email protected]>
@styfle styfle mentioned this pull request Jul 5, 2023
Merged
timneutkens pushed a commit to vercel/next.js that referenced this pull request Jul 6, 2023
@styfle @huozhi
chore(deps): bump react to 7118f5dd7 (#52282)
6dafc42 
Fixes #49409

### React upstream changes

- facebook/react#27045
- facebook/react#27051
- facebook/react#27032
- facebook/react#27031
- facebook/react#27029
- facebook/react#27028
- facebook/react#27027
- facebook/react#27019
- facebook/react#26954
- facebook/react#26987
- facebook/react#26985
- facebook/react#26933
- facebook/react#26625
- facebook/react#27011
- facebook/react#27008
- facebook/react#26997
- facebook/react#26989
- facebook/react#26955
- facebook/react#26963
- facebook/react#26983
- facebook/react#26914
- facebook/react#26951
- facebook/react#26977
- facebook/react#26958
- facebook/react#26940
- facebook/react#26939
- facebook/react#26887
- facebook/react#26947
- facebook/react#26945
- facebook/react#26942
- facebook/react#26938
- facebook/react#26844
- facebook/react#25510
- facebook/react#26932
- facebook/react#26896
- facebook/react#26913
- facebook/react#26888
- facebook/react#26827
- facebook/react#26889
- facebook/react#26877
- facebook/react#26873
- facebook/react#26880
- facebook/react#26842
- facebook/react#26858
- facebook/react#26754
- facebook/react#26753
- facebook/react#26881

---------

Co-authored-by: Jiachi Liu <[email protected]>
shuding added a commit to vercel/next.js that referenced this pull request Jul 8, 2023
@styfle @shuding
chore(deps): bump react 18.3.0-canary-1fdacbefd-20230630 (#52005)
1ae3f42 
This PR updates the vendored react dependencies using `pnpm sync-react`

### React upstream changes

- facebook/react#27028
- facebook/react#27027
- facebook/react#27019
- facebook/react#26954
- facebook/react#26987
- facebook/react#26985
- facebook/react#26933
- facebook/react#26625
- facebook/react#27011
- facebook/react#27008
- facebook/react#26997
- facebook/react#26989
- facebook/react#26955
- facebook/react#26963
- facebook/react#26983
- facebook/react#26914
- facebook/react#26951
- facebook/react#26977
- facebook/react#26958
- facebook/react#26940
- facebook/react#26939
- facebook/react#26887
- facebook/react#26947
- facebook/react#26945
- facebook/react#26942
- facebook/react#26938
- facebook/react#26844
- facebook/react#25510
- facebook/react#26932
- facebook/react#26896
- facebook/react#26913
- facebook/react#26888
- facebook/react#26827
- facebook/react#26889
- facebook/react#26877
- facebook/react#26873
- facebook/react#26880
- facebook/react#26842
- facebook/react#26858
- facebook/react#26754
- facebook/react#26753
- facebook/react#26881

### Related

Closes #49409 (by facebook/react#26977)
fix NEXT-1189

Co-authored-by: Shu Ding <[email protected]>
@eps1lon eps1lon mentioned this pull request Jan 30, 2024
Closed
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
@tyao1 @AndyPengc12
Always trigger componentWillUnmount in StrictMode (facebook#26842)
21817df 
<!--
  Thanks for submitting a pull request!
We appreciate you spending the time to work on these changes. Please
provide enough information so that others can review your pull request.
The three fields below are mandatory.

Before submitting a pull request, please make sure the following is
done:

1. Fork [the repository](https://github.com/facebook/react) and create
your branch from `main`.
  2. Run `yarn` in the repository root.
3. If you've fixed a bug or added code that should be tested, add tests!
4. Ensure the test suite passes (`yarn test`). Tip: `yarn test --watch
TestName` is helpful in development.
5. Run `yarn test --prod` to test in the production environment. It
supports the same options as `yarn test`.
6. If you need a debugger, run `yarn test --debug --watch TestName`,
open `chrome://inspect`, and press "Inspect".
7. Format your code with
[prettier](https://github.com/prettier/prettier) (`yarn prettier`).
8. Make sure your code lints (`yarn lint`). Tip: `yarn linc` to only
check changed files.
  9. Run the [Flow](https://flowtype.org/) type checks (`yarn flow`).
  10. If you haven't already, complete the CLA.

Learn more about contributing:
https://reactjs.org/docs/how-to-contribute.html
-->

## Summary

<!--
Explain the **motivation** for making this change. What existing problem
does the pull request solve?
-->
In StrictMode, React currently only triggers `componentWillUnmount` if
`componentDidMount` is defined. This would miss detecting issues like
initializing resources in constructor or componentWillMount, for
example:
```
class Component {
  constructor() {
     this._subscriptions = new Subscriptions();
  }
  componentWillUnmount() {
     this._subscriptions.reset();
  } 
}
``` 

The PR makes `componentWillUnmount` always run in StrictMode.


## How did you test this change?

<!--
Demonstrate the code is solid. Example: The exact commands you ran and
their output, screenshots / videos if the pull request changes the user
interface.
How exactly did you verify that your PR solves the issue you wanted to
solve?
  If you leave this empty, your PR will very likely be closed.
-->
`yarn test ci`
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
@tyao1
Always trigger componentWillUnmount in StrictMode (#26842)
2c4d9ce 
<!--
  Thanks for submitting a pull request!
We appreciate you spending the time to work on these changes. Please
provide enough information so that others can review your pull request.
The three fields below are mandatory.

Before submitting a pull request, please make sure the following is
done:

1. Fork [the repository](https://github.com/facebook/react) and create
your branch from `main`.
  2. Run `yarn` in the repository root.
3. If you've fixed a bug or added code that should be tested, add tests!
4. Ensure the test suite passes (`yarn test`). Tip: `yarn test --watch
TestName` is helpful in development.
5. Run `yarn test --prod` to test in the production environment. It
supports the same options as `yarn test`.
6. If you need a debugger, run `yarn test --debug --watch TestName`,
open `chrome://inspect`, and press "Inspect".
7. Format your code with
[prettier](https://github.com/prettier/prettier) (`yarn prettier`).
8. Make sure your code lints (`yarn lint`). Tip: `yarn linc` to only
check changed files.
  9. Run the [Flow](https://flowtype.org/) type checks (`yarn flow`).
  10. If you haven't already, complete the CLA.

Learn more about contributing:
https://reactjs.org/docs/how-to-contribute.html
-->

## Summary

<!--
Explain the **motivation** for making this change. What existing problem
does the pull request solve?
-->
In StrictMode, React currently only triggers `componentWillUnmount` if
`componentDidMount` is defined. This would miss detecting issues like
initializing resources in constructor or componentWillMount, for
example:
```
class Component {
  constructor() {
     this._subscriptions = new Subscriptions();
  }
  componentWillUnmount() {
     this._subscriptions.reset();
  }
}
```

The PR makes `componentWillUnmount` always run in StrictMode.

## How did you test this change?

<!--
Demonstrate the code is solid. Example: The exact commands you ran and
their output, screenshots / videos if the pull request changes the user
interface.
How exactly did you verify that your PR solves the issue you wanted to
solve?
  If you leave this empty, your PR will very likely be closed.
-->
`yarn test ci`

DiffTrain build for commit 8110222.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sammy-SC sammy-SC sammy-SC approved these changes

@kassens kassens Awaiting requested review from kassens

@rickhanlonii rickhanlonii Awaiting requested review from rickhanlonii

@gaearon gaearon Awaiting requested review from gaearon

@noahlemen noahlemen Awaiting requested review from noahlemen

Assignees
No one assigned
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tyao1 @sammy-SC @facebook-github-bot