Skip to content

[DevTools] chore: extract getInternalReactConstants#673

Open
everettbu wants to merge 2 commits into
mainfrom
devtools/extract-getInternalReactConstants
Open

[DevTools] chore: extract getInternalReactConstants#673
everettbu wants to merge 2 commits into
mainfrom
devtools/extract-getInternalReactConstants

Conversation

@everettbu

Copy link
Copy Markdown

Mirror of facebook/react#35986
Original author: hoxyq


The test is quite naive, but worth having as additional check for changes in this logic.

@greptile-apps

greptile-apps Bot commented Mar 10, 2026

Copy link
Copy Markdown

Greptile Summary

This PR extracts the getInternalReactConstants function (and its associated types) from renderer.js into a new dedicated module DevToolsFiberInternalReactConstants.js, making it independently importable and testable. The extraction itself is clean — all necessary imports and type exports are correctly set up, and the function logic is unchanged. The PR also includes an unrelated fix in InspectedElementSuspendedBy.js that adds an early-return guard when there are no suspense groups to render.

Key changes:

  • getInternalReactConstants moved verbatim to DevToolsFiberInternalReactConstants.js; types (getDisplayNameForFiberType, getTypeSymbolType, ReactPriorityLevelsType) are now exported from that module
  • renderer.js cleaned up: gt, getDisplayName, getWrappedDisplayName, several React symbol constants, and the WorkTagMap type import are all removed since they were only needed by the extracted function
  • InspectedElementSuspendedBy.js: adds if (groups.length === 0) { return null; } before the main JSX return — this guard is placed after the unknownSuspenders message JSX is computed but before it can be rendered, causing it to be silently suppressed in cases where suspendedBy is empty but unknownSuspenders is a non-NONE reason (e.g. production build, old React version, or thrown-Promise suspension)

Confidence Score: 3/5

  • The extraction refactor is safe, but the InspectedElementSuspendedBy.js change introduces a regression that silently hides unknown-suspender diagnostic messages.
  • The core goal of this PR — extracting getInternalReactConstants — is executed correctly and carries no risk. However, the accompanying change in InspectedElementSuspendedBy.js introduces a logic bug: the new groups.length === 0 early return is placed after the unknownSuspenders message JSX is computed but before it is rendered, meaning users will see a blank panel (null) instead of the helpful "Something threw a Promise…" / production / old-version explanatory messages when there are no specific suspendedBy entries.
  • packages/react-devtools-shared/src/devtools/views/Components/InspectedElementSuspendedBy.js needs attention due to the early return that can swallow unknownSuspenders messages.

Important Files Changed

Filename Overview
packages/react-devtools-shared/src/backend/fiber/shared/DevToolsFiberInternalReactConstants.js New file that cleanly extracts getInternalReactConstants from renderer.js. All necessary imports (symbols, type utilities, gt/gte, WorkTagMap) are correctly brought in. Types are exported so callers can reference them. Logic is identical to what was removed from renderer.js.
packages/react-devtools-shared/src/backend/fiber/renderer.js Correctly removes the getInternalReactConstants function body and cleans up all its now-local imports (gt, getDisplayName, getWrappedDisplayName, moved symbol constants, WorkTagMap type). The new import from DevToolsFiberInternalReactConstants is wired in and used unchanged at the existing call site.
packages/react-devtools-shared/src/devtools/views/Components/InspectedElementSuspendedBy.js Adds an early return null when groups.length === 0, but this guard is placed AFTER the unknownSuspenders JSX is computed, meaning it can silently suppress the "Something threw a Promise…" / production-mode / old-version warning messages when suspendedBy is empty but unknownSuspenders is non-NONE.

Comments Outside Diff (1)

  1. packages/react-devtools-shared/src/devtools/views/Components/InspectedElementSuspendedBy.js, line 587-589 (link)

    Early return silently suppresses unknownSuspenders messages

    groups is built purely from suspendedBy — it is empty whenever suspendedBy is null or empty. The existing guard at line 466 only returns early when both suspendedBy is empty and unknownSuspenders === UNKNOWN_SUSPENDERS_NONE. When unknownSuspenders carries a real reason (e.g. UNKNOWN_SUSPENDERS_REASON_THROWN_PROMISE, UNKNOWN_SUSPENDERS_REASON_PRODUCTION, or UNKNOWN_SUSPENDERS_REASON_OLD_VERSION) while suspendedBy is empty, the code correctly falls through that first guard — but then hits this new check and returns null, throwing away the explanatory message that was just built in the switch block above.

    Concrete case that regresses: a component suspended by throwing a raw Promise (no use()) will have unknownSuspenders === UNKNOWN_SUSPENDERS_REASON_THROWN_PROMISE and an empty suspendedBy. The user will now see a blank panel instead of "Something threw a Promise to suspend this boundary…".

    The guard should also verify that no unknown suspender message needs to be shown:

Fix All in Claude Code Fix All in Codex

Prompt To Fix All With AI
This is a comment left during a code review.
Path: packages/react-devtools-shared/src/devtools/views/Components/InspectedElementSuspendedBy.js
Line: 587-589

Comment:
**Early return silently suppresses `unknownSuspenders` messages**

`groups` is built purely from `suspendedBy` — it is empty whenever `suspendedBy` is `null` or empty. The existing guard at line 466 only returns early when *both* `suspendedBy` is empty **and** `unknownSuspenders === UNKNOWN_SUSPENDERS_NONE`. When `unknownSuspenders` carries a real reason (e.g. `UNKNOWN_SUSPENDERS_REASON_THROWN_PROMISE`, `UNKNOWN_SUSPENDERS_REASON_PRODUCTION`, or `UNKNOWN_SUSPENDERS_REASON_OLD_VERSION`) while `suspendedBy` is empty, the code correctly falls through that first guard — but then hits this new check and returns `null`, throwing away the explanatory message that was just built in the `switch` block above.

Concrete case that regresses: a component suspended by throwing a raw Promise (no `use()`) will have `unknownSuspenders === UNKNOWN_SUSPENDERS_REASON_THROWN_PROMISE` and an empty `suspendedBy`. The user will now see a blank panel instead of *"Something threw a Promise to suspend this boundary…"*.

The guard should also verify that no unknown suspender message needs to be shown:

```suggestion
  if (groups.length === 0 && unknownSuspenders === null) {
    return null;
  }
```

How can I resolve this? If you propose a fix, please make it concise.

Last reviewed commit: a6564b0

hoxyq and others added 2 commits March 12, 2026 14:19
The potential paddings and margins for the empty block are already
handled via CSS selectors.
@everettbu
everettbu force-pushed the devtools/extract-getInternalReactConstants branch from 000387c to a6564b0 Compare March 12, 2026 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants