Warn if static flag is accidentally cleared #20807
Merged
+30
−0
Conversation
"Static" fiber flags are flags that are meant to exist for the lifetime of a component. It's really important not to accidentally reset these, because we use them to decide whether or not to perform some operation on a tree (which we can do because they get bubbled via `subtreeFlags)`. We've had several bugs that were caused by this mistake, so we actually don't rely on static flags anywhere, yet. But we'd like to. So let's roll out this warning and see if it fires anywhere. Once we can confirm that there are no warnings, we can assume that it's safe to start using static flags. I did not wrap it behind a feature flag, because it's dev-only, and we can use our internal warning filter to hide this from the console.
This seems a bit optimistic because it assumes the problem is on the paths stressed in development. We could make this a hard error as the next step to verify it never fires in production to gain confidence. |
|
Comparing: 483358c...99dc7a9 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: (No significant changes) |
|
Is confidence worth the cost of crashing people's apps for a bug that theoretically might happen sometimes? We actually managed to ship this bug to 1% of users without causing a terrible crash. |
|
What I would do instead is add production logging to the profiling build |
acdlite
merged commit 696e736
into
facebook:master
12 of 23 checks passed
12 of 23 checks passed
ci/circleci: yarn_test--r=stable --env=development --persistent
CircleCI is running your tests
Details
ci/circleci: yarn_test--r=www-classic --env=development --variant
CircleCI is running your tests
Details
ci/circleci: yarn_test--r=www-modern --env=development --variant
CircleCI is running your tests
Details
ci/circleci: yarn_test--r=www-classic --env=production --variant
Your tests passed on CircleCI!
Details
ci/circleci: yarn_test--r=www-modern --env=production --variant
Your tests passed on CircleCI!
Details
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
"Static" fiber flags are flags that are meant to exist for the lifetime of a component. It's really important not to accidentally reset these, because we use them to decide whether or not to perform some operation on a tree (which we can do because they get bubbled via
subtreeFlags).We've had several bugs that were caused by this mistake, so we actually don't rely on static flags anywhere, yet. But we'd like to.
So let's roll out this warning and see if it fires anywhere. Once we can confirm that there are no warnings, we can assume that it's safe to start using static flags.
I did not wrap it behind a feature flag, because it's dev-only, and we can use our internal warning filter to hide this from the console.