Clarify that there's three different kinds of OffscreenProps #32838
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is the internal data structure and not the public API.
sebmarkbage
commented
Apr 9, 2025
| // Note: These happen to have identical begin phases, for now. We shouldn't hold | ||
| // ourselves to this constraint, though. If the behavior diverges, we should | ||
| // fork the function. | ||
| // This just works today because it has the same Props. |
There was a problem hiding this comment.
If we want to change the internal format of the OffscreenProps we just have to make the translation here.
|
Comparing: 3fbfb9b...699e4ae 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) |
ActivityProps - Public API LegacyHiddenProps - Public Legacy API OffscreenProps - Internal implementation detail
7540693 to
2ef0a4f
Compare
We should figure out where we should put all these types that are public API like ViewTransitionProps, SuspenseProps etc. They're currently untyped in Fizz.
This was referenced Apr 9, 2025
rickhanlonii
approved these changes
Apr 10, 2025
| @@ -645,8 +647,8 @@ function updateOffscreenComponent( | |||
| current: Fiber | null, | |||
| workInProgress: Fiber, | |||
| renderLanes: Lanes, | |||
| nextProps: OffscreenProps, | |||
There was a problem hiding this comment.
Just curious, what's the thinking here? Better for types?
There was a problem hiding this comment.
This is the implementation detail of both LegacyHidden AND OffscreenComponent which may have different prop types. I pass in both OffscreenProps and LegacyProps in here and so if we change them, then types will break and we'll not forget to bridge LegacyHidden.
sebmarkbage
added a commit
that referenced
this pull request
Apr 10, 2025
Stacked on #32838. We don't always type the Props of built-ins. This adds typing for most of the built-ins. When we did type them, we used to put it in the `ReactFiber...Component` files but any public API like this can be implemented in other renderers too such as Fizz. So I moved them to `shared/ReactTypes` which is where we put other public API types (that are not already built-in to Flow). That way Fizz can import them and assert properly when it accesses the props.
github-actions bot
pushed a commit
that referenced
this pull request
Apr 10, 2025
Stacked on #32838. We don't always type the Props of built-ins. This adds typing for most of the built-ins. When we did type them, we used to put it in the `ReactFiber...Component` files but any public API like this can be implemented in other renderers too such as Fizz. So I moved them to `shared/ReactTypes` which is where we put other public API types (that are not already built-in to Flow). That way Fizz can import them and assert properly when it accesses the props. DiffTrain build for [c44e4a2](c44e4a2)
github-actions bot
pushed a commit
that referenced
this pull request
Apr 10, 2025
Stacked on #32838. We don't always type the Props of built-ins. This adds typing for most of the built-ins. When we did type them, we used to put it in the `ReactFiber...Component` files but any public API like this can be implemented in other renderers too such as Fizz. So I moved them to `shared/ReactTypes` which is where we put other public API types (that are not already built-in to Flow). That way Fizz can import them and assert properly when it accesses the props. DiffTrain build for [c44e4a2](c44e4a2)
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.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ActivityProps - Public API
LegacyHiddenProps - Public Legacy API
OffscreenProps - Internal implementation detail