Dynamic imports using React.lazy should be created outside the render function, otherwise you're creating a new component with every render of JobSpacesList

There isn't really anything 'outside' of render in a FC 😅 . But useMemo should be used around the React.lazy declaration, yea (as it was properly done in saved_objects_table_page.tsx)

Highly depends on the answers on #91976 (comment) though.

I just pushed some changes.

Wherever I could do so (inside the Spaces plugin) I pulled out the lazy loading to the top level.
Otherwise, I memoized these inside of function components.

Suspense will throw an error if the lazy component cannot be loaded so you might want to wrap this in an error boundary and create a fallback.

This component is only rendered if the Spaces plugin is enabled and the Spaces API is available, and the getters to lazy load these components (getSpaceList, getShareToSpaceFlyout) are just promises to import the components that we are already guaranteed to have available. IMO we don't need to complicate this with an error boundary and fallback when we can rely on the Kibana platform's public contracts.

As discussed offline -- this is necessary if the user runs into any network errors at load time 😅 I'll add wrappers for these that display a toast message so the whole page doesn't blow up.

Edit: done in 032b5cb.

I probably don't have enough context (lol, see what I did there?) so might be better to catch up tomorrow but it would be nice to be a bit clearer here in regards to naming.

getSpacesContext to me sounds like we're getting a React Context object for spaces. That's usually a static object which contains a Provider and a Consumer component. In this case however, we are wrapping it in React.lazy to create a lazy loaded component so it's just the Provider component by the looks of it? In that case it might be clearer to call the method getSpacesProvider and the return value SpacesProvider.

Having said that, Context (returned by createContext) is usually a static object in React so we could just make that available through the spaces plugin contract directly rather than having these getter functions and having to memoize them.

Anyways, let's catch up tomorrow, just adding this as a reminder.

Yea, I share the same concerns. React context providers are usually static, and the value they provide mutates. I'm not sure to understand why we need an lazy accessor around a context provider?

it would be nice to be a bit clearer here in regards to naming.

I'm open to naming changes!

getSpacesContext to me sounds like we're getting a React Context object for spaces. That's usually a static object which contains a Provider and a Consumer component. In this case however, we are wrapping it in React.lazy to create a lazy loaded component so it's just the Provider component by the looks of it?

Sort of. It's a "wrapper" component that includes the context provider. We need this because when this wrapper is mounted, it makes a series of API calls and caches the results, then makes those available to its children via the context provider.

The function to obtain this wrapper within the Spaces plugin is called getSpacesContextWrapper, but the one that is exposed to consumers in the public contract is called getSpacesContext. We could rename the latter to something like getSpacesContextProviderWrapper but I don't know what value that really adds for consumers, it's an implementation detail IMO. They don't need to consume the context provider, they just need to know that they have to render the flyout, space list, etc. as a descendant of this wrapper.

Having said that, Context (returned by createContext) is usually a static object in React so we could just make that available through the spaces plugin contract directly rather than having these getter functions and having to memoize them.

I don't think so, we need the wrapper.

I'm not sure to understand why we need an lazy accessor around a context provider?

Because it's not just a context provider, it's the wrapper that also fetches data. Also, to flip that question around: why not provide a lazy accessor? All of the other components have lazy accessors. It seems this approach is more consistent.

Caught up with Joe and I can't think of a good alternative to avoid the architectural complexity with the approach here.

To fulfil the brief, we need a way to make the spaces state available globally, so that we can provide drop-in components for developers that share that state. However, we're not able to render the context provider at the root level of the application ourselves (since spaces is a plugin) and we can't provide static exports (due to the restriction imposed by the platform), hence the indirection using dynamically created and memoized context providers, that require developers to consume them using the spaces plugin contract rather than static imports like EUI.

I'm guessing this is a bit of an edge case and other plugin developers don't have the same requirements to share core functionality like spaces does. However, I would argue that certain things like i18n, routing, session information, authenticated user information and in this particular case spaces are global concerns that should be available throughout the application. Usually these are managed at the root level in React applications so that any child component has access via hooks and context but this isn't really possible with the way we've currently modelled spaces as a plugin.

Happy to leave this as is for now but I am slightly worried about the complexity and maintainability of the current solution.

Again, the side effect here will run at every render unless you add a dependency array and you also need to ensure the component is still mounted when modifying state asynchronously.

What you could do so you don't have to manage all that yourself is use the useAsync hook from react-use module:

const { value: { notifications } } = useAsync(getStartServices); // `value` is the `coreStart` object returned by `getStartServices`

return <SuspenseErrorBoundary notifications={notifications} />

https://streamich.github.io/react-use/?path=/story/side-effects-useasync--docs

Good idea, thanks!
Destructuring makes this a bit funky, but this is what I did to make it work:

  const { value: startServices = [{ notifications: undefined }] } = useAsync(getStartServices);
  const [{ notifications }] = startServices;

For next time: An empty object should be enough here:

const { value: startServices = [{}] } = useAsync(getStartServices);

Ah, I did this because there's no type overlap between CoreStart and {}, so tsc complains about that. Using { notifications: undefined } allows for type inferencing. Now that you mention it though I suppose this would work

const { value: startServices = [{} as CoreStart] } = useAsync(getStartServices);

I think it's a bit better to have explicit types but we've got a big mix of inferences + explicit typing everywhere 🙈