Masterbar: async-load launch button to prevent Explat warnings#109841
Masterbar: async-load launch button to prevent Explat warnings#109841
Conversation
Load MasterbarLaunchButton via AsyncLoad so its chunk is not in the initial masterbar bundle. Export default for AsyncLoad require(). Made-with: Cursor
Jetpack Cloud Live (direct link)
Automattic for Agencies Live (direct link)
Dashboard Live (dotcom) (direct link)
|
|
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
p-jackson
left a comment
There was a problem hiding this comment.
@aaronyan when calypso/lib/explat is imported on the server we get this error message.
It makes sense that we skip initialising this on the server, but we safely skip it by returning null. So what's the purpose of the error message? Afaict nothing bad happens, and in fact I would think skipping this is expected behaviour? If a React component uses explat, then it makes sense to skip this on the server and then when the component is hydrated client side is when the anonID is set up.
Can't we remove this line or at least downgrade it from an error to just an info message?
| } | ||
|
|
||
| return <MasterbarLaunchButton siteId={ siteId } />; | ||
| return <AsyncLoad require="./masterbar-launch-button" placeholder={ null } siteId={ siteId } />; |
There was a problem hiding this comment.
The problem is that AsyncLoad can't be server rendered, it's internal <Suspense> element can't be serialised by vanilla React. I merged #109543 to work around this issue for the help centre icon, but I think we could maybe do a "simpler" solution for the MasterbarLaunchButton?
The issue is that master-bar-launch-button.tsx imports lib/explat, which then sets up the anonId at the module level. No way of avoiding it once you've imported lib/explat.
This is some untested code. We could switch the useExperiment call in master-bar-launch-button.tsx out for a hook like this.
function useAsyncExperiment( experimentName: string ) {
const [ result, setResult ] = useState( [ true, null ] );
useEffect( () => {
let isSubscribed = true;
import ('calypso/lib/explat').then( ( { loadExperimentAssignment } ) => {
if ( isSubscribed ) {
loadExperimentAssignment( experimentName ).then( ( data ) => {
if ( isSubscribed ) {
setResult( [ false, data ] );
}
} );
}
} );
return () => {
isSubscribed = false;
};
}, [ experimentName ] );
return result;
}But even this might be more complicated than necessary. See question in the comment above.
|
Follow up #109849 |
|
Thanks @p-jackson! I'll share some thoughts on your PR. Thanks for fixing this at the ExPlat level. |
Proposed Changes
MasterbarLaunchButtonwithAsyncLoadin the logged-in masterbar instead of a static import.masterbar-launch-button.tsxto a default export so the async chunk resolves correctly.Why are these changes being made?
The masterbar is loaded in SSR contexts and because the Launch Button contains experiment setup code and the explat code should only be run in the browser, it was giving us
Testing Instructions