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: allow custom impl of backend realod-to-profile support check #31048

Merged

Conversation

EdmondChuiHW
Copy link
Contributor

Summary

In preparation to support reload-to-profile in Fusebox (#31021), we need a way to check capability of different backends, e.g. web vs React Native.

How did you test this change?

  • Default, e.g. existing web impl = no-op
  • Custom impl: is called

Copy link

vercel bot commented Sep 24, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-compiler-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 26, 2024 10:36am

@@ -181,8 +181,7 @@ export type BackendEvents = {
fastRefreshScheduled: [],
getSavedPreferences: [],
inspectedElement: [InspectedElementPayload],
isBackendStorageAPISupported: [boolean],
isSynchronousXHRSupported: [boolean],
isReloadAndProfileSupported: [boolean],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can it be backendSupportsReloadAndProfile or isReloadAndProfileSupportedByBackend?

The final version is up to you, I just want it to explicitly mention that this is about backend.

@@ -355,6 +373,7 @@ export function connectWithCustomMessagingProtocol({
}

const agent = new Agent(bridge);
bridge.send('isReloadAndProfileSupported', isReloadAndProfileSupported());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should move it somewhere. Can this actually be an argument on initBackend?

Notice that you don't have any changes related to the browser extension, this is because it has its own logic for backend initialization:

function activateBackend(version: string, hook: DevToolsHook) {
const backend = hook.backends.get(version);
if (!backend) {
throw new Error(`Could not find backend for version "${version}"`);
}
const {Agent, Bridge, initBackend, setupNativeStyleEditor} = backend;
const bridge = new Bridge({
listen(fn) {
const listener = (event: $FlowFixMe) => {
if (
event.source !== window ||
!event.data ||
event.data.source !== 'react-devtools-content-script' ||
!event.data.payload
) {
return;
}
fn(event.data.payload);
};
window.addEventListener('message', listener);
return () => {
window.removeEventListener('message', listener);
};
},
send(event: string, payload: any, transferable?: Array<any>) {
window.postMessage(
{
source: 'react-devtools-bridge',
payload: {event, payload},
},
'*',
transferable,
);
},
});
const agent = new Agent(bridge);
agent.addListener('shutdown', () => {
// If we received 'shutdown' from `agent`, we assume the `bridge` is already shutting down,
// and that caused the 'shutdown' event on the `agent`, so we don't need to call `bridge.shutdown()` here.
hook.emit('shutdown');
});
initBackend(hook, agent, window);
// Setup React Native style editor if a renderer like react-native-web has injected it.
if (typeof setupNativeStyleEditor === 'function' && hook.resolveRNStyle) {
setupNativeStyleEditor(
bridge,
agent,
hook.resolveRNStyle,
hook.nativeStyleEditorValidAttributes,
);
}
// Let the frontend know that the backend has attached listeners and is ready for messages.
// This covers the case of syncing saved values after reloading/navigating while DevTools remain open.
bridge.send('extensionBackendInitialized');
// this backend is activated
requiredBackends.delete(version);
}

Then, inside initBackend, we could do something like:

if (isReloadAndProfileSupported()) {
  agent.onReloadAndProfileSupportedByHost();
}

and inside Agent this method would just send this event:

this._bridge.send('isReloadAndProfileSupported', true);

and the only remaining thing is hardcoding true for initBackend call in the extension code, which I've referenced above

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will allow us to reuse as much as possible, and reduce the fragmentation: instead of having custom logic, like here in react-devtools-core/src/backend.js, we would pass an argument to initBackend, which is from react-devtools-shared

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea! Much cleaner to have this logic back into Agent/initBackend. Updated

@@ -94,7 +95,27 @@ export function initBackend(
}
});

const getIsReloadAndProfileSupported =
hook.settings?.getIsReloadAndProfileSupported ??
Copy link
Contributor

@hoxyq hoxyq Sep 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't do anything, because you don't store it on the Hook object, and you don't need to.

Can you add boolean optional parameter to initBackend here?

export function initBackend(
hook: DevToolsHook,
agent: Agent,
global: Object,
): () => void {

Then, anywhere where you are calling initBackend: browser extension content script, react-devtools-core, react-devtools-inline, you will provide this argument.

For browser extension it will be true by default, for react-devtools-inline it will be false (we don't support it), for react-devtools-core this will be the return of the function isReloadAndProfileSupported, which is passed from the Host (React Native in our case)


export type InitBackend = typeof initBackend;

export function initBackend(
hook: DevToolsHook,
agent: Agent,
global: Object,
getIsReloadAndProfileSupported: () => boolean = defaultGetIsReloadAndProfileSupported,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be boolean with a default value false? And then wrappers around it are going to pass just a boolean value, and react-devtools-core is going to pass getIsReloadAndProfileSupported().

With a current implementation, the extension is going to use defaultGetIsReloadAndProfileSupported, but actually it should just receive a static true value.

Copy link
Contributor Author

@EdmondChuiHW EdmondChuiHW Sep 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't the extension still need to check the local/session storage capabilities, e.g. incognito mode?

Edit: I'm inclined to keep this PR focused on just the refactoring, i.e. keeping existing things working the way it has been, even if it's suboptimal/obsolete. We can follow up and remove the checks in a separate PR as needed

@EdmondChuiHW EdmondChuiHW merged commit f8024b0 into facebook:main Sep 26, 2024
184 checks passed
@EdmondChuiHW EdmondChuiHW deleted the custom-reload2profile-check branch September 26, 2024 11:39
EdmondChuiHW added a commit that referenced this pull request Sep 26, 2024
## Summary

Add reload to profile for Fusebox 

Stacked on #31048. See
6be1977

## How did you test this change?

Test E2E in [D63233256](https://www.internalfb.com/diff/D63233256)
gnoff added a commit to vercel/next.js that referenced this pull request Sep 27, 2024
abhi12299 pushed a commit to abhi12299/next.js that referenced this pull request Sep 29, 2024
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.

3 participants