[DevTools] Support FunctionComponent.contextTypes #19028
Conversation
|
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 745204b:
|
bl00mber
changed the title
Details of bundled changes.Comparing: 38a512a...745204b react-debug-tools
Size changes (experimental) |
Details of bundled changes.Comparing: 38a512a...745204b react-debug-tools
Size changes (stable) |
bl00mber
force-pushed
the
devtools/support-contextTypes
branch
from
7652312 to
f630986
Compare
bl00mber
force-pushed
the
devtools/support-contextTypes
branch
from
f630986 to
33a86ec
Compare
|
This argument is about to be removed from React and replace with other things so not sure it's worth supporting at this point. DevTools would need to fork to try to figure out if it should include it or not. |
|
Maybe add version flags and disable this on renderers > v17 then? |
bl00mber
force-pushed
the
devtools/support-contextTypes
branch
4 times, most recently
from
b19a42c to
36d78b1
Compare
bl00mber
force-pushed
the
devtools/support-contextTypes
branch
from
36d78b1 to
bede31a
Compare
|
Another case is that forwardRef passes a second arg. That kind of doesn't work already but that's probably fine since it needs to be resilient to missing refs anyway. However, it shouldn't pass context to a forwardRef component even if it has contextTypes. (We also have an experimental feature, Blocks, that provides a second arg but I'll remove that one.) |
|
Disabled for all types except FunctionComponent. |
bl00mber
force-pushed
the
devtools/support-contextTypes
branch
from
8aec60d to
882d0df
Compare
bl00mber
changed the title
bl00mber
changed the title
|
Seems like it doesn't merge cleanly? |
|
Updated |
bvaughn
left a comment
bvaughn
left a comment
There was a problem hiding this comment.
I'm concerned by the coupling between the react-debug-tools package and a legacy React feature that only DevTools should support going forward. (This PR does not introduce that coupling. It just highlights it.)
A change like this should probably have several new unit tests as well (for both react-debug-tools and React DevTools). You down to include those with this PR?
I'm going to add test coverage for the DevTools side of this: For now, I'll have to disable the function + legacy context tests though. |
Co-authored-by: Brian Vaughn <[email protected]>
Co-authored-by: Brian Vaughn <[email protected]>
bl00mber
force-pushed
the
devtools/support-contextTypes
branch
from
1689628 to
bc8cddd
Compare
|
Added a test which fails when PR contents are disabled |
| const hasLegacyContext = !!childFiber.elementType.contextTypes; | ||
| if (hasLegacyContext) { |
There was a problem hiding this comment.
Why is the test doing this check? Seems kind of like lower-level implementation details, and could also lead to a false positive (e.g. replace with !!childFiber.elementType.contextTypess above and the test would still pass because the boolean check would just fail silently).
Since we know the legacy context is being used here, why do we need a conditional at all?
The test also doesn't verify that the expected context value gets injected.
There was a problem hiding this comment.
That was my probably inappropriate attempt to prevent test from running on versions where context should be deprecated.
Removed a conditional & added value verification.
bvaughn
left a comment
bvaughn
left a comment
There was a problem hiding this comment.
I think the changes on the DevTools side look fine, but I'm still a little concerned about the added complexity from this changes to react-debug-tools (and the fact that it adds an optional parameter to the exported inspectHooks method that react-debug-tools will need to continue supporting in future releases– unless we break with a major bump).
What are your thoughts about this, @sebmarkbage?
|
This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated. |
|
This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated. |
7 participants
resolve #15039
Since stateless components (FunctionComponent) have no state, legacyContext cannot be extracted from them. The proposed solution is to traverse fiber tree upwards until stateful component is met and extract legacyContext from it.