-
Notifications
You must be signed in to change notification settings - Fork 50.5k
[DevTools] Apply component filters on initial load #35587
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
base: main
Are you sure you want to change the base?
[DevTools] Apply component filters on initial load #35587
Conversation
4cba399 to
a33fa0e
Compare
a33fa0e to
f34c5b6
Compare
hoxyq
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need to save filters in local storage? Maybe it can be removed after these changes?
react/packages/react-devtools-shared/src/utils.js
Lines 489 to 496 in 3031aae
| export function setSavedComponentFilters( | |
| componentFilters: Array<ComponentFilter>, | |
| ): void { | |
| localStorageSetItem( | |
| LOCAL_STORAGE_COMPONENT_FILTER_PREFERENCES_KEY, | |
| JSON.stringify(persistableComponentFilters(componentFilters)), | |
| ); | |
| } |
| savedPreferencesString + | ||
| backendFile.toString() + | ||
| '\n;' + | ||
| backendFile.toString() + | ||
| '\n;' + | ||
| 'ReactDevToolsBackend.initialize();' + | ||
| `ReactDevToolsBackend.initialize(${componentFiltersString});` + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't work?
initialize(...) receives hook settings as a first argument:
react/packages/react-devtools-core/src/backend.js
Lines 63 to 72 in 3031aae
| export function initialize( | |
| maybeSettingsOrSettingsPromise?: | |
| | DevToolsHookSettings | |
| | Promise<DevToolsHookSettings>, | |
| shouldStartProfilingNow: boolean = false, | |
| profilingSettings?: ProfilingSettings, | |
| maybeComponentFiltersOrComponentFiltersPromise?: | |
| | Array<ComponentFilter> | |
| | Promise<Array<ComponentFilter>>, | |
| ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can test standalone in Safari, by opening fixtures/devtools/standalone/index.html.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 43a0ec0 (this PR)
It's relevant for the versions that don't have access to |
Using the localStorage from the frontend has two downsides: - it comes in while we're profiling when we don't support updating filters - it comes in after the initial mount i.e. we always reconcile the full tree twice on initial load Storing it in the backend means data might desync but that complexity is worth avoiding the two issues above. If we get synchronization issues in practice we can revisit.
3031aae to
43a0ec0
Compare
The extension wasn't using the persistested filters until you made changes in the frontend. Now we're using the same approach as the hook settings which means the timings works out to be before React renders. That way we don't have to immediately throw out the devtools backend tree like we usually do when updating component filters.
The alternative would be to use the
savedPreferencesmessages like react-devtools-inline does. react-devtools-inline approach is less complex but would come with two downsides:Storing it in the backend means data might desync but that complexity is worth avoiding the two issues above.
If we get synchronization issues in practice we can revisit.
This removes reliance on
__REACT_DEVTOOLS_COMPONENT_FILTERS__in favor of passing a sync value or Promise toinitializeinstead.Test plan
CleanShot.2026-01-21.at.19.40.04.mp4