[devtools] 1st class support of used Thenables #32989
Conversation
github-actions
bot
added
the
React Core Team
| case 'fulfilled': | ||
| if (showFormattedValue) { | ||
| const formatted = formatDataForPreview(data.value, false); | ||
| return `fulfilled Thenable {${truncateForDisplay(formatted)}}`; |
There was a problem hiding this comment.
Not too much of a fan of fulfilled Thenable {someString}. Open to suggestions.
There was a problem hiding this comment.
If we really want to be accurate about naming, then Thenable is probably the best option, but I would expect it to be Promise in 99.9 % of cases in user land. Both options are okay to me.
There was a problem hiding this comment.
Yeah, same concern here. I'll just use the name logic we use for class_instance unless it's a plain object which is the only case where we fallback to "Thenable".
So you'd actually get "Promise" and when you do subclassing and pre-seed the special properties, you'd also get the right name
| ## Development | ||
|
|
||
| This target should be run in parallel with the `react-devtools-inline` package. The first step then is to run that target following the instructions in the [`react-devtools-inline` README's local development section](https://github.com/facebook/react/tree/main/packages/react-devtools-inline#local-development). | ||
| This target should be run in parallel with the `react-devtools-inline` package. The first step then is to run that target following the instructions in the [`react-devtools-inline` README's local development section](../react-devtools-inline/README.md#local-development). |
There was a problem hiding this comment.
drive-by fix. Links to the local README in vscode and works on GitHub still: https://github.com/eps1lon/react/blob/sebbie/04-22-_devtools_allow_inspection_of_used_thenables/packages/react-devtools-shell/README.md
| case 'null': | ||
| case 'undefined': | ||
| return data; | ||
| return String(data); |
There was a problem hiding this comment.
This was not caught by types since other callsites of formatDataForPreview(undefined) always coerced the return value of formatDataForPreview to a string. But the annotated return type of formatDataForPreview is string.
Hardening types by changing data to mixed is annoying since so many lines rely on data being any.
eps1lon
force-pushed
the
sebbie/04-22-_devtools_allow_inspection_of_used_thenables
branch
from
a939050 to
b140921
Compare
eps1lon
force-pushed
the
sebbie/04-22-_devtools_allow_inspection_of_used_thenables
branch
from
b140921 to
a58ff7c
Compare
| case 'fulfilled': | ||
| if (showFormattedValue) { | ||
| const formatted = formatDataForPreview(data.value, false); | ||
| return `fulfilled Thenable {${truncateForDisplay(formatted)}}`; |
There was a problem hiding this comment.
If we really want to be accurate about naming, then Thenable is probably the best option, but I would expect it to be Promise in 99.9 % of cases in user land. Both options are okay to me.
|
Are you using some custom fonts in your browser? I am wondering what are these weird offsets for properties values text on the screenshots. |
I have the default size in Chrome at "very large" which most UIs don't account for if they try to align other stuff with text via pixels when fonts are in |
Co-authored-by: Ruslan Lesiutin <rdlesyutin@gmail.com>
eps1lon
force-pushed
the
sebbie/04-22-_devtools_allow_inspection_of_used_thenables
branch
from
103a6fb to
4d93417
Compare
eps1lon
deleted the
sebbie/04-22-_devtools_allow_inspection_of_used_thenables
branch
3 participants
Summary
Previously Promises were serialized as normal objects. This worked out for
used Promises just fine:However, this only worked because we treated Promises as objects and enumerated all their properties. This triggered some unactionable warnings in Next.js where some props were both Promises and enumeration of those props triggered warnings.
Now we special case any Thenables and properties of their subclasses that React special cases (
status,valueandreason).We use the same duck-typing logic for detecting Thenables as React.
How did you test this change?
Added cases for all
used Promise types and unused PromisesChecked local build against Next.js app to ensure no more warnings are logged
Previously: