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

DevTools: Read context values from context dependencies #28467

Merged

Conversation

eps1lon
Copy link
Collaborator

@eps1lon eps1lon commented Feb 28, 2024

Summary

Instead of walking up the fiber tree to fill context values from the provider values we now move a cursor along the context dependencies whenever we read context.

This will allow us to read from host specific contexts where we don't have access to the context object e.g. useFormStatus().

For React versions prior to #20890 we still need to walk the fiber tree up to visit context providers since we don't have memoizedValue on the context dependency in this case.

How did you test this change?

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Feb 28, 2024
@react-sizebot
Copy link

react-sizebot commented Feb 28, 2024

Comparing: 0ae2b13...088fb2c

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 177.19 kB 177.19 kB = 55.23 kB 55.23 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 177.72 kB 177.72 kB = 55.55 kB 55.55 kB
facebook-www/ReactDOM-prod.classic.js = 594.67 kB 594.67 kB = 105.04 kB 105.04 kB
facebook-www/ReactDOM-prod.modern.js = 577.93 kB 577.93 kB = 102.10 kB 102.10 kB
oss-experimental/react-debug-tools/cjs/react-debug-tools.development.js +8.31% 29.68 kB 32.15 kB +6.35% 7.64 kB 8.12 kB
oss-stable-semver/react-debug-tools/cjs/react-debug-tools.development.js +8.31% 29.68 kB 32.15 kB +6.35% 7.64 kB 8.12 kB
oss-stable/react-debug-tools/cjs/react-debug-tools.development.js +8.31% 29.68 kB 32.15 kB +6.35% 7.64 kB 8.12 kB
oss-experimental/react-debug-tools/cjs/react-debug-tools.production.min.js +7.12% 9.91 kB 10.62 kB +6.63% 3.36 kB 3.59 kB
oss-stable-semver/react-debug-tools/cjs/react-debug-tools.production.min.js +7.12% 9.91 kB 10.62 kB +6.63% 3.36 kB 3.59 kB
oss-stable/react-debug-tools/cjs/react-debug-tools.production.min.js +7.12% 9.91 kB 10.62 kB +6.63% 3.36 kB 3.59 kB
test_utils/ReactAllWarnings.js Deleted 66.60 kB 0.00 kB Deleted 16.28 kB 0.00 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
oss-experimental/react-debug-tools/cjs/react-debug-tools.development.js +8.31% 29.68 kB 32.15 kB +6.35% 7.64 kB 8.12 kB
oss-stable-semver/react-debug-tools/cjs/react-debug-tools.development.js +8.31% 29.68 kB 32.15 kB +6.35% 7.64 kB 8.12 kB
oss-stable/react-debug-tools/cjs/react-debug-tools.development.js +8.31% 29.68 kB 32.15 kB +6.35% 7.64 kB 8.12 kB
oss-experimental/react-debug-tools/cjs/react-debug-tools.production.min.js +7.12% 9.91 kB 10.62 kB +6.63% 3.36 kB 3.59 kB
oss-stable-semver/react-debug-tools/cjs/react-debug-tools.production.min.js +7.12% 9.91 kB 10.62 kB +6.63% 3.36 kB 3.59 kB
oss-stable/react-debug-tools/cjs/react-debug-tools.production.min.js +7.12% 9.91 kB 10.62 kB +6.63% 3.36 kB 3.59 kB
test_utils/ReactAllWarnings.js Deleted 66.60 kB 0.00 kB Deleted 16.28 kB 0.00 kB

Generated by 🚫 dangerJS against 088fb2c

}
if (currentFiber === null) {
// Hook inspection without access to the Fiber tree.
return context._currentValue;
Copy link
Collaborator Author

@eps1lon eps1lon Feb 29, 2024

Choose a reason for hiding this comment

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

This is only for ReactDebugTools.inspectHooks where we don't have access to the current fiber (we have tests that would break if we throw just because currentContextDependency is null).

Sebastian Silbermann added 4 commits March 5, 2024 10:36
Neither writing primitive stack cache nor  `ReactDebugTools.inspectHooks()` have
access to the fiber tree.
@eps1lon eps1lon force-pushed the fix/devtools/readContext-context-dependency branch from 583e634 to 0775186 Compare March 5, 2024 09:36
Copy link
Contributor

@hoxyq hoxyq left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good from the DevTools side, but maybe also worth waiting for a review from @sebmarkbage

@eps1lon
Copy link
Collaborator Author

eps1lon commented Mar 13, 2024

Needs to be adjusted to work for React 16.8. Currently throws with Uncaught Error: Cannot read properties of undefined (reading 'firstContext')

@eps1lon eps1lon force-pushed the fix/devtools/readContext-context-dependency branch 2 times, most recently from 11d346a to ccf1548 Compare March 13, 2024 13:31
@eps1lon eps1lon force-pushed the fix/devtools/readContext-context-dependency branch from ccf1548 to 0df1e46 Compare March 13, 2024 13:36
eps1lon added 3 commits March 13, 2024 16:19
…izedValue`

I considered polyfilling `memoizedValue` but it seems safer to call this out when we read the context value.
And also to avoid deopts due to using expando properties on the ContextDependency type in old versions.
@eps1lon eps1lon requested a review from hoxyq March 13, 2024 16:05
Copy link
Contributor

@hoxyq hoxyq left a comment

Choose a reason for hiding this comment

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

LGTM! Can you try using @reactVersion pragma for some tests in react-debug-tools?

If yes, then you can try adding test cases that will cover compatibility with previous versions of React?
This is how we run these tests on CI:

react/.circleci/config.yml

Lines 296 to 309 in a870b2d

run_devtools_tests_for_versions:
docker: *docker
environment: *environment
parallelism: *TEST_PARALLELISM
parameters:
version:
type: string
steps:
- checkout
- attach_workspace:
at: .
- setup_node_modules
- run: ./scripts/circleci/download_devtools_regression_build.js << parameters.version >> --replaceBuild
- run: node ./scripts/jest/jest-cli.js --build --project devtools --release-channel=experimental --reactVersion << parameters.version >> --ci

@eps1lon
Copy link
Collaborator Author

eps1lon commented Mar 14, 2024

If yes, then you can try adding test cases that will cover compatibility with previous versions of React?

Problem is that the inspectedElement-test is broken. I quickly looked at it and as far as I can tell, the whole test setup is broken. It uses the specified React versions for both the inspected tree and what Devtools uses. But Devtools uses its bundled React version so the test should ensure that Devtools uses the React version from the repo, while the inspected React tree uses the specified version. Since the test setup uses the specified React version for both, the test crashes since Devtools uses newer React features from the experimental release channel (e.g. useCashRefresh).

We also already have tests for context inspection. They just don't run in the first place:

// TODO(hoxyq): Enable this test for versions ~18, currently broken
// @reactVersion <= 18.2
xit('should inspect hooks for components that only use context (legacy render)', async () => {

@eps1lon eps1lon merged commit 5cec48e into facebook:main Mar 20, 2024
38 checks passed
@eps1lon eps1lon deleted the fix/devtools/readContext-context-dependency branch March 20, 2024 11:16
gnoff added a commit to gnoff/next.js that referenced this pull request Mar 25, 2024
- facebook/react#28596
- facebook/react#28625
- facebook/react#28616
- facebook/react#28491
- facebook/react#28583
- facebook/react#28427
- facebook/react#28613
- facebook/react#28599
- facebook/react#28611
- facebook/react#28610
- facebook/react#28606
- facebook/react#28598
- facebook/react#28549
- facebook/react#28557
- facebook/react#28467
- facebook/react#28591
- facebook/react#28459
- facebook/react#28590
- facebook/react#28564
- facebook/react#28582
- facebook/react#28579
- facebook/react#28578
- facebook/react#28521
- facebook/react#28550
- facebook/react#28576
- facebook/react#28577
- facebook/react#28571
- facebook/react#28572
- facebook/react#28560
- facebook/react#28569
- facebook/react#28573
- facebook/react#28546
- facebook/react#28568
- facebook/react#28562
- facebook/react#28566
- facebook/react#28565
- facebook/react#28559
- facebook/react#28508
- facebook/react#20432
- facebook/react#28555
- facebook/react#24730
- facebook/react#28472
- facebook/react#27991
- facebook/react#28514
- facebook/react#28548
- facebook/react#28526
- facebook/react#28515
- facebook/react#28533
- facebook/react#28532
- facebook/react#28531
- facebook/react#28407
- facebook/react#28522
- facebook/react#28538
- facebook/react#28509
- facebook/react#28534
- facebook/react#28527
- facebook/react#28528
- facebook/react#28519
- facebook/react#28411
- facebook/react#28520
- facebook/react#28518
- facebook/react#28493
- facebook/react#28504
- facebook/react#28499
- facebook/react#28501
- facebook/react#28496
- facebook/react#28471
- facebook/react#28351
- facebook/react#28486
- facebook/react#28490
- facebook/react#28488
- facebook/react#28468
- facebook/react#28321
- facebook/react#28477
- facebook/react#28479
- facebook/react#28480
- facebook/react#28478
- facebook/react#28464
- facebook/react#28475
- facebook/react#28456
- facebook/react#28319
- facebook/react#28345
- facebook/react#28337
- facebook/react#28335
- facebook/react#28466
- facebook/react#28462
- facebook/react#28322
- facebook/react#28444
- facebook/react#28448
- facebook/react#28449
- facebook/react#28446
- facebook/react#28447
- facebook/react#24580
- facebook/react#28514
- facebook/react#28548
- facebook/react#28526
- facebook/react#28515
- facebook/react#28533
- facebook/react#28532
- facebook/react#28531
- facebook/react#28407
- facebook/react#28522
- facebook/react#28538
- facebook/react#28509
- facebook/react#28534
- facebook/react#28527
- facebook/react#28528
- facebook/react#28519
- facebook/react#28411
- facebook/react#28520
- facebook/react#28518
- facebook/react#28493
- facebook/react#28504
- facebook/react#28499
- facebook/react#28501
- facebook/react#28496
- facebook/react#28471
- facebook/react#28351
- facebook/react#28486
- facebook/react#28490
- facebook/react#28488
- facebook/react#28468
- facebook/react#28321
- facebook/react#28477
- facebook/react#28479
- facebook/react#28480
- facebook/react#28478
- facebook/react#28464
- facebook/react#28475
- facebook/react#28456
- facebook/react#28319
- facebook/react#28345
- facebook/react#28337
- facebook/react#28335
- facebook/react#28466
- facebook/react#28462
- facebook/react#28322
- facebook/react#28444
- facebook/react#28448
- facebook/react#28449
- facebook/react#28446
- facebook/react#28447
- facebook/react#24580
gnoff added a commit to gnoff/next.js that referenced this pull request Mar 25, 2024
- facebook/react#28596
- facebook/react#28625
- facebook/react#28616
- facebook/react#28491
- facebook/react#28583
- facebook/react#28427
- facebook/react#28613
- facebook/react#28599
- facebook/react#28611
- facebook/react#28610
- facebook/react#28606
- facebook/react#28598
- facebook/react#28549
- facebook/react#28557
- facebook/react#28467
- facebook/react#28591
- facebook/react#28459
- facebook/react#28590
- facebook/react#28564
- facebook/react#28582
- facebook/react#28579
- facebook/react#28578
- facebook/react#28521
- facebook/react#28550
- facebook/react#28576
- facebook/react#28577
- facebook/react#28571
- facebook/react#28572
- facebook/react#28560
- facebook/react#28569
- facebook/react#28573
- facebook/react#28546
- facebook/react#28568
- facebook/react#28562
- facebook/react#28566
- facebook/react#28565
- facebook/react#28559
- facebook/react#28508
- facebook/react#20432
- facebook/react#28555
- facebook/react#24730
- facebook/react#28472
- facebook/react#27991
- facebook/react#28514
- facebook/react#28548
- facebook/react#28526
- facebook/react#28515
- facebook/react#28533
- facebook/react#28532
- facebook/react#28531
- facebook/react#28407
- facebook/react#28522
- facebook/react#28538
- facebook/react#28509
- facebook/react#28534
- facebook/react#28527
- facebook/react#28528
- facebook/react#28519
- facebook/react#28411
- facebook/react#28520
- facebook/react#28518
- facebook/react#28493
- facebook/react#28504
- facebook/react#28499
- facebook/react#28501
- facebook/react#28496
- facebook/react#28471
- facebook/react#28351
- facebook/react#28486
- facebook/react#28490
- facebook/react#28488
- facebook/react#28468
- facebook/react#28321
- facebook/react#28477
- facebook/react#28479
- facebook/react#28480
- facebook/react#28478
- facebook/react#28464
- facebook/react#28475
- facebook/react#28456
- facebook/react#28319
- facebook/react#28345
- facebook/react#28337
- facebook/react#28335
- facebook/react#28466
- facebook/react#28462
- facebook/react#28322
- facebook/react#28444
- facebook/react#28448
- facebook/react#28449
- facebook/react#28446
- facebook/react#28447
- facebook/react#24580
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
eps1lon added a commit to vercel/next.js that referenced this pull request Apr 19, 2024
### React upstream changes

- facebook/react#28643
- facebook/react#28628
- facebook/react#28361
- facebook/react#28513
- facebook/react#28299
- facebook/react#28617
- facebook/react#28618
- facebook/react#28621
- facebook/react#28614
- facebook/react#28596
- facebook/react#28625
- facebook/react#28616
- facebook/react#28491
- facebook/react#28583
- facebook/react#28427
- facebook/react#28613
- facebook/react#28599
- facebook/react#28611
- facebook/react#28610
- facebook/react#28606
- facebook/react#28598
- facebook/react#28549
- facebook/react#28557
- facebook/react#28467
- facebook/react#28591
- facebook/react#28459
- facebook/react#28590
- facebook/react#28564
- facebook/react#28582
- facebook/react#28579
- facebook/react#28578
- facebook/react#28521
- facebook/react#28550
- facebook/react#28576
- facebook/react#28577
- facebook/react#28571
- facebook/react#28572
- facebook/react#28560
- facebook/react#28569
- facebook/react#28573
- facebook/react#28546
- facebook/react#28568
- facebook/react#28562
- facebook/react#28566
- facebook/react#28565
- facebook/react#28559
- facebook/react#28508
- facebook/react#20432
- facebook/react#28555
- facebook/react#24730
- facebook/react#28472
- facebook/react#27991
- facebook/react#28514
- facebook/react#28548
- facebook/react#28526
- facebook/react#28515
- facebook/react#28533
- facebook/react#28532
- facebook/react#28531
- facebook/react#28407
- facebook/react#28522
- facebook/react#28538
- facebook/react#28509
- facebook/react#28534
- facebook/react#28527
- facebook/react#28528
- facebook/react#28519
- facebook/react#28411
- facebook/react#28520
- facebook/react#28518
- facebook/react#28493
- facebook/react#28504
- facebook/react#28499
- facebook/react#28501
- facebook/react#28496
- facebook/react#28471
- facebook/react#28351
- facebook/react#28486
- facebook/react#28490
- facebook/react#28488
- facebook/react#28468
- facebook/react#28321
- facebook/react#28477
- facebook/react#28479
- facebook/react#28480
- facebook/react#28478
- facebook/react#28464
- facebook/react#28475
- facebook/react#28456
- facebook/react#28319
- facebook/react#28345
- facebook/react#28337
- facebook/react#28335
- facebook/react#28466
- facebook/react#28462
- facebook/react#28322
- facebook/react#28444
- facebook/react#28448
- facebook/react#28449
- facebook/react#28446
- facebook/react#28447
- facebook/react#24580
- facebook/react#28514
- facebook/react#28548
- facebook/react#28526
- facebook/react#28515
- facebook/react#28533
- facebook/react#28532
- facebook/react#28531
- facebook/react#28407
- facebook/react#28522
- facebook/react#28538
- facebook/react#28509
- facebook/react#28534
- facebook/react#28527
- facebook/react#28528
- facebook/react#28519
- facebook/react#28411
- facebook/react#28520
- facebook/react#28518
- facebook/react#28493
- facebook/react#28504
- facebook/react#28499
- facebook/react#28501
- facebook/react#28496
- facebook/react#28471
- facebook/react#28351
- facebook/react#28486
- facebook/react#28490
- facebook/react#28488
- facebook/react#28468
- facebook/react#28321
- facebook/react#28477
- facebook/react#28479
- facebook/react#28480
- facebook/react#28478
- facebook/react#28464
- facebook/react#28475
- facebook/react#28456
- facebook/react#28319
- facebook/react#28345
- facebook/react#28337
- facebook/react#28335
- facebook/react#28466
- facebook/react#28462
- facebook/react#28322
- facebook/react#28444
- facebook/react#28448
- facebook/react#28449
- facebook/react#28446
- facebook/react#28447
- facebook/react#24580
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team React 19
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants