Skip to content

Commit d6f9de5

Browse files
tyao1tyao1
committed
Always trigger componentWillUnmount in StrictMode (#26842)
<!-- 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)
1 parent 9908c7d commit d6f9de5

19 files changed

+1658
-1706
lines changed

‎compiled/facebook-www/REVISION‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
018c58c9c65452cff25aaf1f38f78a9b90d8e5c1
1+
811022232efed62a3c943701bc99d18655bc78b3

‎compiled/facebook-www/React-dev.modern.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ if (
2727
}
2828
"use strict";
2929

30-
var ReactVersion = "18.3.0-www-modern-d087d75f";
30+
var ReactVersion = "18.3.0-www-modern-a6d62651";
3131

3232
// ATTENTION
3333
// When adding new symbols to this file,

‎compiled/facebook-www/React-prod.modern.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -641,4 +641,4 @@ exports.useSyncExternalStore = function (
641641
);
642642
};
643643
exports.useTransition = useTransition;
644-
exports.version = "18.3.0-www-modern-fb2ee8b7";
644+
exports.version = "18.3.0-www-modern-916dbaab";

‎compiled/facebook-www/ReactART-dev.classic.js‎

Lines changed: 23 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ function _assertThisInitialized(self) {
6969
return self;
7070
}
7171

72-
var ReactVersion = "18.3.0-www-classic-603b8e54";
72+
var ReactVersion = "18.3.0-www-classic-3fed7141";
7373

7474
var LegacyRoot = 0;
7575
var ConcurrentRoot = 1;
@@ -12590,13 +12590,11 @@ function mountClassInstance(workInProgress, ctor, newProps, renderLanes) {
1259012590
}
1259112591

1259212592
if (typeof instance.componentDidMount === "function") {
12593-
var fiberFlags = Update | LayoutStatic;
12594-
12595-
if ((workInProgress.mode & StrictEffectsMode) !== NoMode) {
12596-
fiberFlags |= MountLayoutDev;
12597-
}
12593+
workInProgress.flags |= Update | LayoutStatic;
12594+
}
1259812595

12599-
workInProgress.flags |= fiberFlags;
12596+
if ((workInProgress.mode & StrictEffectsMode) !== NoMode) {
12597+
workInProgress.flags |= MountLayoutDev;
1260012598
}
1260112599
}
1260212600

@@ -12658,13 +12656,11 @@ function resumeMountClassInstance(workInProgress, ctor, newProps, renderLanes) {
1265812656
// If an update was already in progress, we should schedule an Update
1265912657
// effect even though we're bailing out, so that cWU/cDU are called.
1266012658
if (typeof instance.componentDidMount === "function") {
12661-
var fiberFlags = Update | LayoutStatic;
12662-
12663-
if ((workInProgress.mode & StrictEffectsMode) !== NoMode) {
12664-
fiberFlags |= MountLayoutDev;
12665-
}
12659+
workInProgress.flags |= Update | LayoutStatic;
12660+
}
1266612661

12667-
workInProgress.flags |= fiberFlags;
12662+
if ((workInProgress.mode & StrictEffectsMode) !== NoMode) {
12663+
workInProgress.flags |= MountLayoutDev;
1266812664
}
1266912665

1267012666
return false;
@@ -12710,25 +12706,21 @@ function resumeMountClassInstance(workInProgress, ctor, newProps, renderLanes) {
1271012706
}
1271112707

1271212708
if (typeof instance.componentDidMount === "function") {
12713-
var _fiberFlags = Update | LayoutStatic;
12714-
12715-
if ((workInProgress.mode & StrictEffectsMode) !== NoMode) {
12716-
_fiberFlags |= MountLayoutDev;
12717-
}
12709+
workInProgress.flags |= Update | LayoutStatic;
12710+
}
1271812711

12719-
workInProgress.flags |= _fiberFlags;
12712+
if ((workInProgress.mode & StrictEffectsMode) !== NoMode) {
12713+
workInProgress.flags |= MountLayoutDev;
1272012714
}
1272112715
} else {
1272212716
// If an update was already in progress, we should schedule an Update
1272312717
// effect even though we're bailing out, so that cWU/cDU are called.
1272412718
if (typeof instance.componentDidMount === "function") {
12725-
var _fiberFlags2 = Update | LayoutStatic;
12726-
12727-
if ((workInProgress.mode & StrictEffectsMode) !== NoMode) {
12728-
_fiberFlags2 |= MountLayoutDev;
12729-
}
12719+
workInProgress.flags |= Update | LayoutStatic;
12720+
}
1273012721

12731-
workInProgress.flags |= _fiberFlags2;
12722+
if ((workInProgress.mode & StrictEffectsMode) !== NoMode) {
12723+
workInProgress.flags |= MountLayoutDev;
1273212724
} // If shouldComponentUpdate returned false, we should still update the
1273312725
// memoized state to indicate that this work can be reused.
1273412726

@@ -23789,10 +23781,12 @@ function invokeLayoutEffectMountInDEV(fiber) {
2378923781
case ClassComponent: {
2379023782
var instance = fiber.stateNode;
2379123783

23792-
try {
23793-
instance.componentDidMount();
23794-
} catch (error) {
23795-
captureCommitPhaseError(fiber, fiber.return, error);
23784+
if (typeof instance.componentDidMount === "function") {
23785+
try {
23786+
instance.componentDidMount();
23787+
} catch (error) {
23788+
captureCommitPhaseError(fiber, fiber.return, error);
23789+
}
2379623790
}
2379723791

2379823792
break;

‎compiled/facebook-www/ReactART-dev.modern.js‎

Lines changed: 23 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ function _assertThisInitialized(self) {
6969
return self;
7070
}
7171

72-
var ReactVersion = "18.3.0-www-modern-d087d75f";
72+
var ReactVersion = "18.3.0-www-modern-a6d62651";
7373

7474
var LegacyRoot = 0;
7575
var ConcurrentRoot = 1;
@@ -12322,13 +12322,11 @@ function mountClassInstance(workInProgress, ctor, newProps, renderLanes) {
1232212322
}
1232312323

1232412324
if (typeof instance.componentDidMount === "function") {
12325-
var fiberFlags = Update | LayoutStatic;
12326-
12327-
if ((workInProgress.mode & StrictEffectsMode) !== NoMode) {
12328-
fiberFlags |= MountLayoutDev;
12329-
}
12325+
workInProgress.flags |= Update | LayoutStatic;
12326+
}
1233012327

12331-
workInProgress.flags |= fiberFlags;
12328+
if ((workInProgress.mode & StrictEffectsMode) !== NoMode) {
12329+
workInProgress.flags |= MountLayoutDev;
1233212330
}
1233312331
}
1233412332

@@ -12383,13 +12381,11 @@ function resumeMountClassInstance(workInProgress, ctor, newProps, renderLanes) {
1238312381
// If an update was already in progress, we should schedule an Update
1238412382
// effect even though we're bailing out, so that cWU/cDU are called.
1238512383
if (typeof instance.componentDidMount === "function") {
12386-
var fiberFlags = Update | LayoutStatic;
12387-
12388-
if ((workInProgress.mode & StrictEffectsMode) !== NoMode) {
12389-
fiberFlags |= MountLayoutDev;
12390-
}
12384+
workInProgress.flags |= Update | LayoutStatic;
12385+
}
1239112386

12392-
workInProgress.flags |= fiberFlags;
12387+
if ((workInProgress.mode & StrictEffectsMode) !== NoMode) {
12388+
workInProgress.flags |= MountLayoutDev;
1239312389
}
1239412390

1239512391
return false;
@@ -12435,25 +12431,21 @@ function resumeMountClassInstance(workInProgress, ctor, newProps, renderLanes) {
1243512431
}
1243612432

1243712433
if (typeof instance.componentDidMount === "function") {
12438-
var _fiberFlags = Update | LayoutStatic;
12439-
12440-
if ((workInProgress.mode & StrictEffectsMode) !== NoMode) {
12441-
_fiberFlags |= MountLayoutDev;
12442-
}
12434+
workInProgress.flags |= Update | LayoutStatic;
12435+
}
1244312436

12444-
workInProgress.flags |= _fiberFlags;
12437+
if ((workInProgress.mode & StrictEffectsMode) !== NoMode) {
12438+
workInProgress.flags |= MountLayoutDev;
1244512439
}
1244612440
} else {
1244712441
// If an update was already in progress, we should schedule an Update
1244812442
// effect even though we're bailing out, so that cWU/cDU are called.
1244912443
if (typeof instance.componentDidMount === "function") {
12450-
var _fiberFlags2 = Update | LayoutStatic;
12451-
12452-
if ((workInProgress.mode & StrictEffectsMode) !== NoMode) {
12453-
_fiberFlags2 |= MountLayoutDev;
12454-
}
12444+
workInProgress.flags |= Update | LayoutStatic;
12445+
}
1245512446

12456-
workInProgress.flags |= _fiberFlags2;
12447+
if ((workInProgress.mode & StrictEffectsMode) !== NoMode) {
12448+
workInProgress.flags |= MountLayoutDev;
1245712449
} // If shouldComponentUpdate returned false, we should still update the
1245812450
// memoized state to indicate that this work can be reused.
1245912451

@@ -23454,10 +23446,12 @@ function invokeLayoutEffectMountInDEV(fiber) {
2345423446
case ClassComponent: {
2345523447
var instance = fiber.stateNode;
2345623448

23457-
try {
23458-
instance.componentDidMount();
23459-
} catch (error) {
23460-
captureCommitPhaseError(fiber, fiber.return, error);
23449+
if (typeof instance.componentDidMount === "function") {
23450+
try {
23451+
instance.componentDidMount();
23452+
} catch (error) {
23453+
captureCommitPhaseError(fiber, fiber.return, error);
23454+
}
2346123455
}
2346223456

2346323457
break;

0 commit comments

Comments
 (0)