Skip to content

refactor[react-devtools]: rewrite context menus #29049

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
May 20, 2024

Conversation

hoxyq
Copy link
Contributor

@hoxyq hoxyq commented May 13, 2024

Summary

  • While rolling out RDT 5.2.0 on Fusebox, we've discovered that context menus don't work well with this environment. The reason for it is the context menu state implementation - in a global context we define a map of registered context menus, basically what is shown at the moment (see deleted Contexts.js file). These maps are not invalidated on each re-initialization of DevTools frontend, since the bundle (react-devtools-fusebox module) is not reloaded, and this results into RDT throwing an error that some context menu was already registered.
  • We should not keep such data in a global state, since there is no guarantee that this will be invalidated with each re-initialization of DevTools (like with browser extension, for example).
  • The new implementation is based on a ContextMenuContainer component, which will add all required contextmenu event listeners to the anchor-element. This component will also receive a list of items that will be displayed in the shown context menu.
  • The ContextMenuContainer component is also using useImperativeHandle hook to extend the instance of the component, so context menus can be managed imperatively via ref: contextMenu.current?.hide(), for example.
  • Changed: The option for copying value to clipboard is now hidden for functions. The reasons for it are:
    • It is broken in the current implementation, because we call JSON.stringify on the value, see packages/react-devtools-shared/src/backend/utils.js.
    • I don't see any reasonable value in doing this for the user, since Go to definition option is available and you can inspect the real code and then copy it.
    • We already filter out fields from objects, if their value is a function, because the whole object is passed to JSON.stringify.

How did you test this change?

Works with element props and hooks:

  • All context menu items work reliably for props items
  • All context menu items work reliably or hooks items
Screen.Recording.2024-05-13.at.14.12.24.mov

Works with timeline profiler:

  • All context menu items work reliably: copying, zooming, ...
  • Context menu automatically closes on the scroll event
Screen.Recording.2024-05-13.at.14.16.28.mov

Works with Fusebox:

  • Produces no errors
  • Copy to clipboard context menu item works reliably
rdt-fusebox.mov

@hoxyq hoxyq requested a review from motiz88 May 13, 2024 13:05
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels May 13, 2024
@hoxyq hoxyq force-pushed the react-devtools/rewrite-context-menu branch 4 times, most recently from c2d359a to 2af7ca4 Compare May 13, 2024 16:01
@hoxyq hoxyq force-pushed the react-devtools/rewrite-context-menu branch from 2af7ca4 to 9e5edab Compare May 13, 2024 16:07
@hoxyq hoxyq merged commit d14ce51 into facebook:main May 20, 2024
38 checks passed
@hoxyq hoxyq deleted the react-devtools/rewrite-context-menu branch May 20, 2024 14:12
hoxyq added a commit that referenced this pull request Jun 18, 2024
Full list of changes:

* chore[react-devtools]: improve console arguments formatting before
passing it to original console ([hoxyq](https://github.com/hoxyq) in
[#29873](#29873))
* chore[react-devtools]: unify console patching and default to ansi
escape symbols ([hoxyq](https://github.com/hoxyq) in
[#29869](#29869))
* chore[react-devtools/backend]: remove
consoleManagedByDevToolsDuringStrictMode
([hoxyq](https://github.com/hoxyq) in
[#29856](#29856))
* chore[react-devtools/extensions]: make source maps url relative
([hoxyq](https://github.com/hoxyq) in
[#29886](#29886))
* fix[react-devtools] divided inspecting elements between inspecting do…
([vzaidman](https://github.com/vzaidman) in
[#29885](#29885))
* [Fiber] Create virtual Fiber when an error occurs during reconcilation
([sebmarkbage](https://github.com/sebmarkbage) in
[#29804](#29804))
* fix[react-devtools] component badge in light mode is now not invisible
([vzaidman](https://github.com/vzaidman) in
[#29852](#29852))
* Remove Warning: prefix and toString on console Arguments
([sebmarkbage](https://github.com/sebmarkbage) in
[#29839](#29839))
* Add jest lint rules ([rickhanlonii](https://github.com/rickhanlonii)
in [#29760](#29760))
* [Fiber] Track the Real Fiber for Key Warnings
([sebmarkbage](https://github.com/sebmarkbage) in
[#29791](#29791))
* fix[react-devtools/store-test]: fork the test to represent current be…
([hoxyq](https://github.com/hoxyq) in
[#29777](#29777))
* Default native inspections config false
([vzaidman](https://github.com/vzaidman) in
[#29784](#29784))
* fix[react-devtools] remove native inspection button when it can't be
used ([vzaidman](https://github.com/vzaidman) in
[#29779](#29779))
* chore[react-devtools]: ip => internal-ip
([hoxyq](https://github.com/hoxyq) in
[#29772](#29772))
* Fix #29724: `ip` dependency update for CVE-2024-29415
([Rekl0w](https://github.com/Rekl0w) in
[#29725](#29725))
* cleanup[react-devtools]: remove unused supportsProfiling flag from
store config ([hoxyq](https://github.com/hoxyq) in
[#29193](#29193))
* [Fiber] Enable Native console.createTask Stacks When Available
([sebmarkbage](https://github.com/sebmarkbage) in
[#29223](#29223))
* Move createElement/JSX Warnings into the Renderer
([sebmarkbage](https://github.com/sebmarkbage) in
[#29088](#29088))
* Set the current fiber to the source of the error during error
reporting ([sebmarkbage](https://github.com/sebmarkbage) in
[#29044](#29044))
* Unify ReactFiberCurrentOwner and ReactCurrentFiber
([sebmarkbage](https://github.com/sebmarkbage) in
[#29038](#29038))
* Dim `console` calls on additional Effect invocations due to
`StrictMode` ([eps1lon](https://github.com/eps1lon) in
[#29007](#29007))
* refactor[react-devtools]: rewrite context menus
([hoxyq](https://github.com/hoxyq) in
[#29049](#29049))
# for free to join this conversation on GitHub. Already have an account? # 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.

3 participants