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

Support Module Federation + React SSR #926

Closed
brunos3d opened this issue Nov 29, 2022 · 14 comments
Closed

Support Module Federation + React SSR #926

brunos3d opened this issue Nov 29, 2022 · 14 comments
Assignees

Comments

@brunos3d
Copy link

🐛 Bug Report

There are problems when trying to integrate the loadable with a React application that uses Module Federation, especially if the application performs SSR with federated components.

Sample to reproduce

I created a repository that exemplifies the issues in two use cases, one with React 16 and the other with React 18. Here is the repo link

https://github.com/brunos3d/loadable-module-federation-react-ssr

How to run it

To make it work just clone it, navigate with the terminal to one of the example folders, install the dependencies with yarn, and then run yarn start, here are the commands to run quickly:

cd samples/loadable-react-16
yarn install
yarn start

About the sample repo

The project has a readme file that explains how to use it, but basically, there are two workspaces in the samples folder, two applications in each workspace.

Expected behavior

In both samples the expected result is the same: render the app1 on the server side consuming the Content component that is federated by app2 application using Module Federation and importing it using loadable. The component must be delivered to the client side statically and must be hydrated so that it can be reactive whenever the user changes the text input.

Workarounds and hacks

I worked alongside @ScriptedAlchemy so that we could identify a way to make this work and we created some workarounds in the examples to make this work. They are described/listed here, I hope that these applied hacks can be useful to solve the problem natively. The places that have the application of these workarounds are marked with the following comment // ================ WORKAROUND ================.

@ScriptedAlchemy
Copy link

@theKashey So the issue here is that loadable is using require.resolveWeak, we need a way server-side to "flush chunks" in such a manner that the federated require call still exists.

If we do if(notbrowser) {require('app2/thing')} and loadable(import(app2/thing)), then loadable works server side since webpack sees the require call as an eager import and will hold startup till those additional chunks are loaded.

Would it be possible to add a federated: true flag to loadable which in turn injects a require() instead of require.resolveWeak on a case-by-case basis?

@theKashey
Copy link
Collaborator

To clarify - there is no need to change existing code, only add extra explicit require into server bundle. I am correct?

@theKashey theKashey self-assigned this Nov 30, 2022
@ScriptedAlchemy
Copy link

ScriptedAlchemy commented Nov 30, 2022

To get us unblocked, through the least path of resistance - that's the idea.
ResolveWeak tells webpack "chunk is already in the graph" - in the case of a federated import, webpack is aware that it cannot boot the application without first having the sync import already resolved (the federated module) but since loadable was primarily designed for inter-graph module calling - require.resolveWeak doesn't have the module ID since nobody has 'require()' it to begin with.

While Id like to keep the discussion going, perhaps on another thread about better integration of chunk flushing between host remote pairs - we can make it functional by explicitly requiring the federated module and let webpack internally handle all the preloading that we normally have to make lodable and limitChunkCount plugin do.

So if babel did something like this, just for server builds.

// samples/loadable-react-16/app1/src/client/components/App.tsx

const LoadableContent = loadable(() => import('app2/Content'), {
  fallback: <div>loading content...</div>,
});

if (typeof window === 'undefined') {
  require('app2/Content');
}

More precisely:

import loadable from '@loadable/component'
const OtherComponent = loadable({
  chunkName() {
    return 'App1-Content' // need a solve to map this to remove federated chunk stats, but out of scope
  },
  isReady(props) {
    if (typeof __webpack_modules__ !== 'undefined') {
      return !!__webpack_modules__[this.resolve(props)]
    }
    return false
  },
  requireAsync: () =>
    import(/* webpackChunkName: "OtherComponent" */
    'App2/Content'),
  requireSync(props) {
    const id = this.resolve(props)
    if (typeof __webpack_require__ !== 'undefined') {
      return __webpack_require__(id)
    }
    return eval('module.require')(id)
  },
  resolve(isFederated) {
if(isFederated) {
  return require('App2/Content') // the original import requested, webpack now "sees" it and will delay startup till chunk is ready In host. 
}
    if (require.resolveWeak) {
      return require.resolveWeak('App2/Content')
    }
    return require('path').resolve(__dirname, 'App2/Content')
  },
})

@ScriptedAlchemy
Copy link

Perhaps a magic comment marker could be used if you need a way to parse and know the difference between a internal normal module and a remote imported module.

but the idea i had for users would be

loadable(import('App2/Content'), {federated: true});

Which would produce a transpiled loadable function that uses require instead of require ResolveWeak.

Im not sure if requireSync also needs to be adjusted, but we should be able to test and fiddle with it quite quickly if it does require any additional adjustment. I do believe that given I can just require() it manually in the file - as long as we put require somewhere, webpack will pick it up and ensure that App2/Content is already preloaded before the host entry point is allowed to call its entry point module.

@ScriptedAlchemy
Copy link

Its been a while since my universal days, but do you have a way to know if loadable is processing common.js or browserjs builds during transform? This works for the server, but the reason we even used resolveWeak was to prevent webpack from un-splitting the client bundle.
We could do dead code elimination if we need it, so babel transforms and if(!LOADABLE_SERVER) could be transformed by DefinePlugin to false so webpack removes the reference - so we still get async loading in browser but front/preloading of federated modules on server where Suspense is not yet possible

@theKashey
Copy link
Collaborator

Ok, I’ve got the idea - Webpack should have remotes loaded before it can handle stuff contained in them.
Would an explicit command to load all known loadables be enough, the same way clientside is waiting for stuff to be loaded?
Ie ‘ chunkExtractor.requireEntrypoint({federated:true})’

question:

  • why you talk about sync requires. Should it be dynamic import I have to await for?

@ScriptedAlchemy
Copy link

if you can wait for the dynamic import then yeah that will work - if it needs to be "sync" like resolveWeak does - then it can be require. But if you can await that dynamic import then we can absolutely use it.

Yes webpack will or should load its remotes required upfront or as needed as long as it is aware of them (sees import or require) in the tree.

The issue I ran into with loadable was there's technically two parts, the remoteEntry.js file and the remote chunk you want, so webpack can hold for both but in how loadable works right now we would have to ensure there's some location where federated imports are required in some way, not weak resolved. If loadable can wait on a promise then we can just return import() otherwise we would make a sync require instead of a resolveWeak

@bwhitty
Copy link

bwhitty commented Nov 30, 2022

Would using a sync require() have implications on runtime performance?

e.g. a require() would mean that all this must be done up-front (at app bootup), but a import() would mean that we'd be able to resolve federated imports at any point during runtime with no impact on the running app's process (via the non-blocking import)

@ScriptedAlchemy
Copy link

ScriptedAlchemy commented Dec 1, 2022

Yeah a sync require would mean a blocking import - however, this has often been the case for non suspense react render tools. ResolveWeak is sync because we force the chunks back into the main chunk in order to ensure everything is there that needs to be before attempting a single render pass. We can await a async import like Kashey suggested but this is still a blocking operation because no element can be a promise once the render begins.
With that said, we can mitigate concerns around the overhead of pulling federated code over the network, depending on your setup.

At lululemon we are going to use EFS like a mounted volume in the VPC so MF can read a "disk" instead of fetch a string from S3, since s3 is way slower and efs has tested to take like 5ms to pull a 1mb chunk.

If you're on persistent servers like docker, there's more options to tamp down this delaying response times.

If we do a pre-pass render to flush out used imports, that's also an option but I'd suggest benchmarking to validate that in effort to pull modules a bit lazier, we don't end up adding more execution overhead.

This is the problem with sync renders. Before we can walk the tree at all, anything that might be in the tree has to be ready.

However, how I designed node federation is to behave like an async file read when require is called. So if we can use import(), the upfront cost would be reduced and when the file is actually needed - the webpack runtime can read the file on demand.
There is still the potential for some block time somewhere tho because eventually we must return a sync module graph, at least as far as react is concerned.
At the webpack runtime level we can be lazier.

We can also strike a balance on the chunking algorithms. So if preload is required, we are not loading like 50 little files - but can tell webpack to join small chunks together so there's less to pull.

I'd rather pull 10 10kb files than 100 1kb files. Server side we are not as limited on network and bandwidth, so pulling say 1mb - the http handshake probably is the majority of the delay and the download is extremely fast.

I'd also weigh up the longevity of sync renders at your company.
Could some block time if need be be acceptable for a shorter period of time if the general idea is to unlock more powerful architectures that are not possible without a solution right now. Is it a years long mission to move the important apps to v18 if the host is v18 then it's nonblocking even with v17 remotes since the apis match up.

Lastly, I do think wall time can be reduced quite a bit if we were to handle multiple async parts at once. Like fetch data + preload modules + some other async. Then promise all it.

I've done this with nested render passes in the past and most the time it was my api calls and secrets manager that took longer. So by the time all the other parts were done, MF was already ready.
Still you'd only be as fast as the slowest promise, but you could do as much work as possible in parallel so it's not a waterfall

In node Federation require is fs.readFile or fetch + vm.runInThisContext, so as long as the remote is loaded - requiring other code, webpack won't know the difference between reading the FS or fetching a chunk since both are promise that resolves string to webpack. Within the app it's "sync" - but what depends is how dependency management is setup, sync require is hoisted, import() is not which would give async handling back to webpack without the framework having any idea that code is being pulled by webpack runtime

@ScriptedAlchemy
Copy link

Kashey came up with the concept of preheating, the idea being you promise all some deps then on next tick, kick start the render.
Which reduces hammering memory pressure all at once.

All this said, if we can use import() and await it in loadable, we can optimize or at least allow the application to kickstart, just not start rendering.

For things like data loading that would be perfect, webpack can warm up while the application is busy fetching route level data since data must resolve before render can begin

@brunos3d
Copy link
Author

brunos3d commented Dec 9, 2022

Hello folks, just sharing something we can add to our update. I just discovered that apollo-client found a way to create a global context and avoid duplication of contexts, this is a problem that happens with loadable in our example project here.

We can use this strategy with our ChunkExtractorManager to pass the ChunkExtractor instance properly to the InnerLoadable in the withChunkExtractor function component using the ChunkExtractor.collectChunks without workarounds.

image

cc: @theKashey @ScriptedAlchemy

@theKashey
Copy link
Collaborator

Not sure if I follow as the following code is equal

// https://github.com/gregberge/loadable-components/blob/3ec9b6400972c5a8bcd860ffbb9199dabaa2186d/packages/server/src/ChunkExtractor.js#L380
  collectChunks(app) {
    return <ChunkExtractorManager extractor={this}>{app}</ChunkExtractorManager>
  }

// https://github.com/brunos3d/loadable-module-federation-react-ssr/blob/3826ae58aae70fe63314d1f3163b68b8009d76f0/samples/loadable-react-16/app1/src/server/renderAndExtractContext.tsx#L31-L41
  const markup = renderToString(
    <ChunkExtractorManager {...{ extractor: chunkExtractor }}>
      <App />,
    </ChunkExtractorManager>,
  );

PS: watch for line ending in your case, there is some unexpected stuff (,)

@brunos3d
Copy link
Author

brunos3d commented Dec 14, 2022

Hello, folks. Me and @ScriptedAlchemy just created a PR that adds a new option to babel-plugin called moduleFederation, it makes Module Federation compatible with loadable.

Here is a branch with the changes to test this fix.

cc: @theKashey @ScriptedAlchemy

@stale
Copy link

stale bot commented Apr 2, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Apr 2, 2023
@stale stale bot closed this as completed Aug 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants