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: Unwrap Promise in useFormState #28319

Merged
merged 5 commits into from
Feb 28, 2024

Conversation

eps1lon
Copy link
Collaborator

@eps1lon eps1lon commented Feb 13, 2024

Summary

Instead of just showing plain "Promise" after an async action, we now unwrap the fulfilled value of the Promise.

How did you test this change?

  • added case to devtools fixture:

    Screen.Recording.2024-02-13.at.17.49.29.mov

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Feb 13, 2024
Comment on lines 520 to 540
// If this was an uncached Promise we have to abandon this attempt
// but we can still emit anything up until this point.
hookLog.push({
primitive: 'Unresolved',
stackError: new Error(),
value: thenable,
debugInfo:
thenable._debugInfo === undefined ? null : thenable._debugInfo,
});
throw SuspenseException;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Followed the same pattern as #28297.

Do we actually reach this with useFormStatus? Asking mainly because SuspenseException currently includes a message that's specific to use

Copy link
Collaborator

Choose a reason for hiding this comment

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

Even if instrumented, the promise could still be pending.

@react-sizebot
Copy link

react-sizebot commented Feb 13, 2024

Comparing: 2e470a7...adea250

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 = 176.83 kB 176.83 kB = 55.11 kB 55.11 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 178.83 kB 178.83 kB = 55.69 kB 55.69 kB
facebook-www/ReactDOM-prod.classic.js = 592.18 kB 592.18 kB = 104.43 kB 104.43 kB
facebook-www/ReactDOM-prod.modern.js = 575.46 kB 575.46 kB = 101.43 kB 101.43 kB
oss-experimental/react-debug-tools/cjs/react-debug-tools.development.js +4.36% 27.66 kB 28.87 kB +2.69% 7.36 kB 7.56 kB
oss-stable-semver/react-debug-tools/cjs/react-debug-tools.development.js +4.36% 27.66 kB 28.87 kB +2.69% 7.36 kB 7.56 kB
oss-stable/react-debug-tools/cjs/react-debug-tools.development.js +4.36% 27.66 kB 28.87 kB +2.69% 7.36 kB 7.56 kB
oss-experimental/react-debug-tools/cjs/react-debug-tools.production.min.js +3.43% 9.07 kB 9.38 kB +2.89% 3.21 kB 3.31 kB
oss-stable-semver/react-debug-tools/cjs/react-debug-tools.production.min.js +3.43% 9.07 kB 9.38 kB +2.89% 3.21 kB 3.31 kB
oss-stable/react-debug-tools/cjs/react-debug-tools.production.min.js +3.43% 9.07 kB 9.38 kB +2.89% 3.21 kB 3.31 kB
test_utils/ReactAllWarnings.js Deleted 66.26 kB 0.00 kB Deleted 16.32 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 +4.36% 27.66 kB 28.87 kB +2.69% 7.36 kB 7.56 kB
oss-stable-semver/react-debug-tools/cjs/react-debug-tools.development.js +4.36% 27.66 kB 28.87 kB +2.69% 7.36 kB 7.56 kB
oss-stable/react-debug-tools/cjs/react-debug-tools.development.js +4.36% 27.66 kB 28.87 kB +2.69% 7.36 kB 7.56 kB
oss-experimental/react-debug-tools/cjs/react-debug-tools.production.min.js +3.43% 9.07 kB 9.38 kB +2.89% 3.21 kB 3.31 kB
oss-stable-semver/react-debug-tools/cjs/react-debug-tools.production.min.js +3.43% 9.07 kB 9.38 kB +2.89% 3.21 kB 3.31 kB
oss-stable/react-debug-tools/cjs/react-debug-tools.production.min.js +3.43% 9.07 kB 9.38 kB +2.89% 3.21 kB 3.31 kB
test_utils/ReactAllWarnings.js Deleted 66.26 kB 0.00 kB Deleted 16.32 kB 0.00 kB

Generated by 🚫 dangerJS against adea250

@eps1lon eps1lon marked this pull request as ready for review February 13, 2024 16:56
// If this was an uncached Promise we have to abandon this attempt
// but we can still emit anything up until this point.
hookLog.push({
primitive: 'Unresolved',
Copy link
Collaborator

@sebmarkbage sebmarkbage Feb 13, 2024

Choose a reason for hiding this comment

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

This needs a different name and you need to exhaust this case in the stack frame cache. See https://github.com/facebook/react/blob/main/packages/react-debug-tools/src/ReactDebugHooks.js#L111-L117

Otherwise this will generate the wrong stack frame information and so it'll generate a messed up tree.

Probably should add some tests for it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Did that just like use but it's not clear to me how I could test it. It was working fine in the fixture without the case as far as I could tell.

I guess I would need to simulate an update in getPrimitiveStackCache because we never actually call useFormState with a thenable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This needs a different name

Can it just use FormState? I don't understand the purpose of using different primitive names for the same hook.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's annoying because this is used both for the key in the cache map and each one will have different stack traces. So it needs to be unique per stack. However, it's also used for the display name.

Which is why I gave it a unique primitive but then aliased it back:

https://github.com/facebook/react/blob/main/packages/react-debug-tools/src/ReactDebugHooks.js#L795

We could just make two separate fields for the name and the key potentially.

Since both your cases are in the same Hook you can also just unify them so that they have the same new Error().stack and ideally the same hookLog.push(...).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unified stack error and hook log. I have to fight flow analysis more when unifying the hook log push more unless I tie up value and error into a single type like { value: Awaited<S>, error: null } | { value: Thenable, error: mixed }.

@eps1lon eps1lon force-pushed the feat/devtools/unwrap-useformstate branch 2 times, most recently from f1738f4 to 13c058a Compare February 13, 2024 22:36
@eps1lon eps1lon changed the title Devtools: Unwrap Promise in useFormStatus Devtools: Unwrap Promise in useFormState Feb 15, 2024
@eps1lon eps1lon force-pushed the feat/devtools/unwrap-useformstate branch from 80ad00b to d3c3fdb Compare February 15, 2024 10:19
@eps1lon eps1lon force-pushed the feat/devtools/unwrap-useformstate branch from d3c3fdb to adea250 Compare February 15, 2024 10:34
@eps1lon eps1lon merged commit fb10a2c into facebook:main Feb 28, 2024
36 checks passed
@eps1lon eps1lon deleted the feat/devtools/unwrap-useformstate branch February 28, 2024 22:53
hoxyq added a commit that referenced this pull request Mar 5, 2024
* feat[devtools]: symbolicate source for inspected element
([hoxyq](https://github.com/hoxyq) in
[#28471](#28471))
* refactor[devtools]: lazily define source for fiber based on component
stacks ([hoxyq](https://github.com/hoxyq) in
[#28351](#28351))
* fix[devtools/tree/element]: onClick -> onMouseDown to handle first
click correctly ([hoxyq](https://github.com/hoxyq) in
[#28486](#28486))
* [DOM] disable legacy mode behind flag
([gnoff](https://github.com/gnoff) in
[#28468](#28468))
* Fix Broken Links In Documentation
([justindhillon](https://github.com/justindhillon) in
[#28321](#28321))
* Update /link URLs to react.dev
([rickhanlonii](https://github.com/rickhanlonii) in
[#28477](#28477))
* [tests] add support for @GATE pragma
([gnoff](https://github.com/gnoff) in
[#28479](#28479))
* Devtools: Unwrap Promise in useFormState
([eps1lon](https://github.com/eps1lon) in
[#28319](#28319))
* Add support for rendering BigInt
([eps1lon](https://github.com/eps1lon) in
[#24580](#24580))
* Include server component names in the componentStack in DEV
([sebmarkbage](https://github.com/sebmarkbage) in
[#28415](#28415))
eps1lon pushed a commit to rickhanlonii/react that referenced this pull request Mar 5, 2024
Basically just port facebook#28319
to avoid merge conflicts of this PR with main
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
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
* feat[devtools]: symbolicate source for inspected element
([hoxyq](https://github.com/hoxyq) in
[facebook#28471](facebook#28471))
* refactor[devtools]: lazily define source for fiber based on component
stacks ([hoxyq](https://github.com/hoxyq) in
[facebook#28351](facebook#28351))
* fix[devtools/tree/element]: onClick -> onMouseDown to handle first
click correctly ([hoxyq](https://github.com/hoxyq) in
[facebook#28486](facebook#28486))
* [DOM] disable legacy mode behind flag
([gnoff](https://github.com/gnoff) in
[facebook#28468](facebook#28468))
* Fix Broken Links In Documentation
([justindhillon](https://github.com/justindhillon) in
[facebook#28321](facebook#28321))
* Update /link URLs to react.dev
([rickhanlonii](https://github.com/rickhanlonii) in
[facebook#28477](facebook#28477))
* [tests] add support for @GATE pragma
([gnoff](https://github.com/gnoff) in
[facebook#28479](facebook#28479))
* Devtools: Unwrap Promise in useFormState
([eps1lon](https://github.com/eps1lon) in
[facebook#28319](facebook#28319))
* Add support for rendering BigInt
([eps1lon](https://github.com/eps1lon) in
[facebook#24580](facebook#24580))
* Include server component names in the componentStack in DEV
([sebmarkbage](https://github.com/sebmarkbage) in
[facebook#28415](facebook#28415))
bigfootjon pushed a commit that referenced this pull request Apr 18, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants