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

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

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
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.

None yet

3 participants