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.

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

Regression test: Missing unmount after re-order #20285

Merged
merged 1 commit into from
Nov 18, 2020

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Nov 18, 2020

Adds a regression test for a bug I found in the effects refactor.

The bug was that reordering a child that contains passive effects would cause the child to "forget" that it contains passive effects. This is because when a Placement effect is scheduled by the reconciler, it would override all of the fiber's flags, including its "static" ones:

child.flags = Placement;

The problem is that we use a static flag to use a "static" flag to track that a fiber contains passive effects.

So what happens is that when the tree is deleted, the unmount effect is never fired.

In the new implementation, the fix is to add the Placement flag without overriding the rest of the bitmask:

child.flags |= Placement;

(The old implementation doesn't need to be changed because it does not use static flags for this purpose.)

Adds a regression test for a bug I found in the effects refactor.

The bug was that reordering a child that contains passive effects would
cause the child to "forget" that it contains passive effects. This is
because when a Placement effect is scheduled by the reconciler, it would
override all of the fiber's flags, including its "static" ones:

```
child.flags = Placement;
```

The problem is that we use a static flag to use a "static" flag to track
that a fiber contains passive effects.

So what happens is that when the tree is deleted, the unmount effect is
never fired.

In the new implementation, the fix is to add the Placement flag without
overriding the rest of the bitmask:

```
child.flags |= Placement;
```

(The old implementation doesn't need to be changed because it does not
use static flags for this purpose.)
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Nov 18, 2020
@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 7426d00:

Sandbox Source
React Configuration

@sizebot
Copy link

sizebot commented Nov 18, 2020

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against 7426d00

@sizebot
Copy link

sizebot commented Nov 18, 2020

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against 7426d00

@acdlite acdlite requested a review from rickhanlonii November 18, 2020 06:16
Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

Nice test!

Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

Nice test!

Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

Nice test!

Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

Nice test!

@acdlite acdlite merged commit 0f83a64 into facebook:master Nov 18, 2020
@sophiebits
Copy link
Collaborator

Nice test!

@bvaughn
Copy link
Contributor

bvaughn commented Nov 18, 2020

That was a GitHub bug 😅 I was only excited enough about the test to comment twice.

@myspivey
Copy link

Nice test!

@facebook facebook locked and limited conversation to collaborators Nov 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants