useAreModulesReady improvements

[tjosepo]

I believe there are a few ways to improve the usability of the useAreModulesReady() hook.

Replacing polling with an observer

Currently, the useAreModulesReady hook uses setInterval to know when module registration is done. The default timeout is 100ms.

Polling is bad for performance for multiple reasons. Setting the interval too low will make the device waste compute time. Setting the interval too high will hurt the time it takes for the app to load.

Using polling is completely unecessary. We can use the Observer pattern to get rid of polling.

Suggested changes

ℹ️ There changes are mostly internal and should not affect the developers using this library.

LocalModuleRegistry

We can add a addRemoteModulesReadyListener(callback: () => void): void function to the module. This function should fire all registed callback methods when the registration status of remote modules is set to "ready".

We should also add a function removeRemoteModulesReadyListener(callback: () => void): void to unregister the callback. This can be useful if the React component listening to the event gets unmounted.

useAreModulesReady()

This hook should use the useSyncExternalStore hook.

function subscribe(callback: () => void) {
  // Add listener
  addRemoteModulesReadyListener(callback);
  
  // Unregister listener on unmount
  return () => removeRemoteModulesReadyListener(callback);
}

function useAreRemoteModulesReady() {
  return useSyncExternalStore(subscribe, getRemoteModuleRegistrationStatus, false);
}

Support Suspense

Supporting Suspense will allow developers use the same Suspense fallback for both loading modules and waiting for their pages to load. This is critical when the loading component is animated (i.e. a loading spinner) to avoid a jitter when the remote modules are loaded, but the individual pages still need to load.

The loading spinner jumps to its initial position halfway during the load, when Squide is done loading and React Router starts loading the route

Suggested changes

I suggest the creation of a new hook useSuspenseAreModulesReady that causes a component to suspend until the modules are loaded.

Using useAreModulesReady

function AppRouter() {
  useSuspenseAreModulesReady(); // this will suspend until ready
  
  const routes = useRoutes();
  const router = useMemo(() => {
    return createBrowserRouter(routes);
  }, [routes]);
 
  return <RouterProvider router={router} /> // this will also suspend when loading routes
}

const root = createRoot(document.getElementById("root"));
root.render(
  <Suspense fallback={<MyBigSpinner />}>
    <AppRouter /> {/* Both Squide and React Router will suspend and use the same fallback */}
  </Suspense>
);

Using Firefly's AppRouter

import { AppRouter } from "@squide/firefly";

const root = createRoot(document.getElementById("root"));
root.render(
  <Suspense fallback={<MyBigSpinner />}>
    <AppRouter /> {/* Both Firefly and React Router will suspend and use the same fallback */}
  </Suspense>
);

Passing a fallbackElement to Firefly's AppRouter

We can force the existance of a fallback element by requiring a prop.

Internally, we should still use the same <Suspense> boundary for both Squide loading the remote modules and React Router loading the routes, otherwise we'll end up with the jitter.

import { AppRouter } from "@squide/firefly";

const root = createRoot(document.getElementById("root"));
root.render(
  <AppRouter fallbackElement={<MyBigSpinner />} /> {/* Both Firefly and React Router will suspend and use the same fallback */}
);

---

That's all I can think of right now. There's a lot of Squide internals that is still unknown to me, so let me know if this feasable or not.

These are very nitpicky suggestions, but I still wanted to document them.

Thank you for your time 🙇

[patricklafrance]

I converted this to a conversation, hope you don't mind!

[patricklafrance]

Thank you for the proposition. A couple of things that comes to mind following my first read...

Polling is bad for performance for multiple reasons. Setting the interval too low will make the device waste compute time. Setting the interval too high will hurt the time it takes for the app to load.

While you are totally right that pooling is bad for performance, I am not sure that it matters for this case as the polling happens for a very short (and kind of predictable) period.

Suggested changes

There are a few things to I could add regarding this section but maybe that would be better if you review the suggested changes first:

• The subtitle is "LocalModuleRegistry" which makes me things that you want to propose a change for the "local module" but then you suggest adding a addRemoteModulesReadyListener(callback: () => void): void function.
• useAreModulesReady() must asses that both "local modules" and "remote modules" are ready but the proposed changes only asses that the remote modules are ready. Is this ommission intended?
• What about useAreModulesRegistered ?

Support Suspense

Supporting Suspense will allow developers use the same Suspense fallback for both loading modules and waiting for their pages to load.

I am not sure why developers would want to do that. Both cannot happen at the same time. A page cannot be loaded until all the modules are registered as there's currently no mechanism to know the requested page is included in which module.

While I agree that we could work on a more sophisticated mechanism to support eager rendering of a page, I don't think this is something worth working on at the moment. Such a mecanism would encounter a few bumps:

• In development, the mechanism would also need to assert that the page request handlers has been loaded
• What if some parts of the page (or the layout) depends on initial data (like the session) ?
• Deferred navigation items would be missing from the rendered navigation
• The pages of the modules that remains to be registered and the deferred pages would not be part of the router. I believe React Router expect to publish an API that programmatically register routes but it's not available at the moment

Let me know if I am missing something, maybe I didn't understand correctly what you meant here.

I would also like to understand how Suspense would integrated with the AppRouter data hooks: onLoadPublicData and onLoadProtectedData.

A unique loader (fallback) should be rendered until:

• Modules are registered and ready
• Initial and protected data loading is completed (protected data is only loaded if the page is protected)
• MSW request handlers are registered (when in development)

[tjosepo]

Polling

The subtitle is "LocalModuleRegistry" which makes me things that you want to propose a change for the "local module" but then you suggest adding a addRemoteModulesReadyListener(callback: () => void): void function.

I indeed mixed up the two. 😅

The suggested change can apply to both LocalModuleRegistry and RemoteModuleRegistry.

useAreModulesReady() must asses that both "local modules" and "remote modules" are ready but the proposed changes only asses that the remote modules are ready. Is this ommission intended?

I omitted local modules for brievity. We can most likely apply the same changes as for the remote modules.

What about useAreModulesRegistered ?

I haven't used it yet, but from my understanding, my suggested changes can also apply to it.

Suspense

I'm not interested in using <Suspense> to render a page eagerly before we all the data we need.

I'm only interested in using <Suspense> as a global fallback UI so we don't need to declare a fallback UI for Squide and another fallback UI for React Router. Declaring two fallback UIs breaks animations as was shown in the GIF in my previous message.

I haven't used onLoadPublicData and onLoadProtectedData yet. Suspense might break those hooks because it seems they're implemented by comparing a previous state with a subsequent state, which makes the component unpure. My gut feeling is that there's probably a better way to do what they are doing - either by loading the data only when it becomes important to the UI or loading it outside of React entirely.

What happens if the registration is completed before a React component has time to declare onLoadPublicData? Is the function going to be called?

It's late so I'm posting this as is. If I haven't everything from your previous comment, feel free to re-ask. Sorry about that. 🙇

[patricklafrance]

I'm only interested in using as a global fallback UI so we don't need to declare a fallback UI for Squide and another fallback UI for React Router. Declaring two fallback UIs breaks animations as was shown in the GIF in my previous message.

Currently, it's the same fallback that is used for the initial data loading and the module registration/ready state. According to my tests, it's not necessary to add a fallback element for the React Router as it's preferably to scope a Suspense boundary inside a layout wrapping an Outlet component. You can have a look at the endpoints sample for such an example.

I haven't used onLoadPublicData and onLoadProtectedData yet. Suspense might break those hooks because it seems they're implemented by comparing a previous state with a subsequent state, which makes the component unpure.

I don't think this is how it works, have a look at the code and let me know what you find out.

What happens if the registration is completed before a React component has time to declare onLoadPublicData? Is the function going to be called?

I don't think it can happen with the current sequence.

Here's my take:

• To be fully transparent, I knew when implementing the AppRouter component that it wasn't the best "React" implementation because I currently lack proper knowledge about Suspense, useExternalAsync and all that stuff, so there might be a more Reactish version to do all this.
• I think that we could quickly implement the improvements that you proposed to remove the interval from the useAreModulesReady and useAreModulesRegistered hooks.
• I feel like the Suspense improvements need more thinking thought, but I am open to reasses these improvements later on. To move on, I would like to have a better understanding of how the data loaders could be integrated in this new paradigm.

[patricklafrance]

The #127 PR introduced the usage of useSyncExternalStore for the useAreModulesRegistered, useAreModulesReady and useIsMswStarted hooks.

[patricklafrance]

#181 refactor the AppRouter component to delegate most responsabilities to smaller hooks.