-
-
Notifications
You must be signed in to change notification settings - Fork 380
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
feat: add moduleFederation option #938
base: main
Are you sure you want to change the base?
feat: add moduleFederation option #938
Conversation
packages/babel-plugin/src/index.js
Outdated
}) | ||
return imports | ||
} | ||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The golden rule of development is to never refactor and change simultaneously.
Look like this file is not changed, however from the git point of view - everything changed.
I also cannot spot any change, but that does not mean there isn't any.
While I do appreciate typings and docs
- please remove unrelated changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, thanks for the tip! Done 🚀
properties.some( | ||
prop => | ||
prop.isObjectProperty() && | ||
prop.get('key').isIdentifier({ name: 'federated' }) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how this work and when it should be used.
Please keep in mind that in no circumstances we can use federated
template on the client-side bundle - that will effectively disable code splitting.
Right now, there is a big probability that it will not happen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, make sense, the idea now is to use moduleFederation: true
option only on server-side compiler
Per my understanding, the essence of this change is to disable code splitting for federation, not to "support" federation keeping the current constraints. The question is how to ensure this configuration is used properly - only for server-side bundle. |
1c056ca
to
6c24c14
Compare
woud we set a DefinePlugin like 'process.isBrowser' and if its false webpack could remove the dead code? Not sure if that would work or not |
@theKashey yeah, your right about using it only on server-side, I will work to make it work as expected, don't have ideas on how we can make it |
@theKashey just removed every unrelated change and forced push it to avoid overwriting the git history, I also removed the federated option, now we need to figure out how to ensure that it uses |
👍 @brunos3d, now the change is short and sound. Define Plugin stuff might now work, as long as in case of What we can do - add a check for browser/clientside/webpack-target and ➡️ throw an error in case of misconfiguration. That would put customers without tests into a little dangerous situation, so we might also not document new property for now |
@theKashey just added the suggested warnings. As I'm not too used to babel/webpack plugins I will ask you for code suggestions to solve this issue without workarounds 😄 |
// eslint-disable-next-line no-console | ||
console.warn( | ||
'It\'s recommended to use "isBrowser" global variable to detect the environment\n' + | ||
'Try to add "webpack.DefinePlugin({ \'process.isBrowser\': true })" to your webpack config', | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there something internal to a "client side" Webpack target
that can be read automatically? For example, as a developer if I set target: "web"
(the default) or another client-targetted value, then this plugin should probably just error out here (or have a "force" option to override).
Or does Webpack not provide a standard way to expose "is browser" and every webpack tool has to invent their own? (like here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One can use package.json:browser
field to detect browser target - https://github.com/theKashey/detect-node/blob/master/package.json#L6-L10
But this is a run-time operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These solutions only work during runtime, @theKashey do you know if webpack provides some context to babel during the compilation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to babel-loader source - no, it does not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, this begs the question then: can this be an argument to LoadablePlugin
webpack plugin instead, and then read somehow by the Babel plugin? Is that possible?
That would alleviate most concerns of misuse, as it could be documented to just use this option for your client-side Webpack build, which is something already natural to loadable components.
With a Babel plugin option, users have to implement some logic to modify their Babel options dynamically based on whether it's being used by the Webpack server build or client build, which while not weird is an additional configuration workflow added to use loadable, and they also have to implement some DefinePlugin
thing, which again is another workflow change.
No big deal, just wondering if that could simplify things
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 BINGO!
LoadablePlugin
can automatically call DefinePlugin
with the variables required. As webpack plugin is expected to be used only for client side it can secure dead code elimination (in production only) and other checks to guide proper configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome @theKashey! I will work on that here! 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@theKashey added DefinePlugin to LoadablePlugin and updated the warning messages
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that there's a webpack plugin integration, what's the purpose of the Babel plugin argument? Is there a use case for a standalone Babel build for Loadable without Webpack? If not, then it seems the Webpack plugin can be the singular configuration point.
I'm thinking that this federation config can now be set up in one place: the server-side Webpack configuration's LoadablePlugin
. Like so:
// webpack.config.server.js
plugins: [
new LoadablePlugin({
serverSideModuleFederation: true
}),
]
Then internally the plugin would set process.serverSideModuleFederation = true
, and then @loadable/babel-plugin
can just read that. There's no need to set up a separate config on the Babel plugin, as it's configured via Webpack.
packages/webpack-plugin/src/index.js
Outdated
const defs = { | ||
'process.isBrowser': | ||
compiler.options.target === 'web' || | ||
compiler.options.target === undefined, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To go with my suggestion of adding a serverSideModuleFederation
argument to the webpack plugin, if we do then this logic could throw an error for misconfiguration:
const defs = { | |
'process.isBrowser': | |
compiler.options.target === 'web' || | |
compiler.options.target === undefined, | |
} | |
if (this.opts.serverSideModuleFederation === true && | |
compiler.options.target === 'web' || | |
compiler.options.target === undefined | |
) { | |
throw new Error(`Enabling server-side module federation support for a client-side Webpack target (${compiler.options.target}) is unnecessary and will effectively disable all code-splitting.` | |
} |
packages/webpack-plugin/src/index.js
Outdated
new ((webpack && webpack.DefinePlugin) || require('webpack').DefinePlugin)( | ||
defs, | ||
).apply(compiler) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with my suggestion this would become
new ((webpack && webpack.DefinePlugin) || require('webpack').DefinePlugin)( | |
defs, | |
).apply(compiler) | |
if (this.opts.serverSideModuleFederation) { | |
// eslint-disable-next-line global-require | |
new ((webpack && webpack.DefinePlugin) || require('webpack').DefinePlugin)({ | |
serverSideModuleFederation: true | |
}).apply(compiler) | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job @bwhitty, I will test it here!! 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@theKashey I am not being able to get the value of serverSideModuleFederation
passed to DefinePlugin during the execution of the resolveProperty function, do you have some idea on how can I access this value?
I already tried with:
- process.serverSideModuleFederation
- global.serverSideModuleFederation
- only serverSideModuleFederation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ScriptedAlchemy I got these snippets from NodeFederationPlugin do you have some idea why I can't get the serverSideModuleFederation during babel compilation execution?
packages/webpack-plugin/src/index.js
Outdated
(this.opts.serverSideModuleFederation === true && | ||
compiler.options.target === 'web') || | ||
compiler.options.target === undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these parens make this logic wrong I think? should be
(this.opts.serverSideModuleFederation === true && | |
compiler.options.target === 'web') || | |
compiler.options.target === undefined | |
this.opts.serverSideModuleFederation === true && | |
(compiler.options.target === 'web' || compiler.options.target === undefined) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it was not correct, just fixed, but it was not the problem
Hey, folks as we didn't find a proper way to pass the |
@theKashey any input here - would be great to move this forwards if we can |
packages/webpack-plugin/src/index.js
Outdated
} | ||
|
||
if (this.opts.serverSideModuleFederation) { | ||
Object.defineProperty(process, 'serverSideModuleFederation', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 that's a fancy way to "teleport" settings.
However, the particular way you did it will break some assumptions of node creating a hard to understand "gap" for a few (rare) users of thread-loader or similar stuff.
Ease to fix. Just store what is required in process.env - a special place designed for this.
Before we processed with the merge, I want to ask one question - Where is the best place to handle this stuff?
|
This does module federation for all modules. Should we do it as a option on loadable call? |
It does, but should be only used on the server-side, in the past I had put it as an option for the loadable function, it was a option called 'federated'. But the problem is, MF plugin changes the way imports happen even non-federated containers get these |
So that is the question: if one technology should adapt to support another technology - could there be a third technology to change as well? |
Proper suspense would work for everything moving forwards, but we would still need a solve for async-node target / lower react versions :P |
@theKashey @ScriptedAlchemy An issue we're facing right now is when we try to consume a container from some remote using loadable and the remote is down, MF tries to generate a fake remote and a fake container, but loadable throws multiple errors saying that the provided module is not a react component (cause it's an empty object), an a bunch of other errors. By now I'm using the following workaround for local linked and patched @loadable/component // on InnerLoadable component at the top of the "render" function
if (result && typeof result === 'object' && Object.keys(result).length === 0) {
console.log('probably the fake container')
result = `<span>failed to load remote container</span>`
}
// ... It checks if the result (the loaded/resolved module) is not undefined and verifies if its an empty object (the fake container generated by MF) then it replaces it with the warning message |
There is a
|
@brunos3d i might be able to help with the fake remote which returns null. We could modify node federation or allow it to throw a rejection. Generally I avoid that since throwing errors in sync imported components can cause webpack runtime to crash |
great work! |
@ranshamay Probably we will not merge this PR, but I'm working on some stuff:
I will post the updates here soon as I finish everything, but its also good to keep an eye on https://valor-software.com/articles too 🚀 |
@brunos3d I saw this example that you prepared: https://github.com/module-federation/module-federation-examples/tree/88c252fbc3fdc3262e0d8e3d03a945276bd0a165/loadable-react-16 In my server-side micro frontend application, I will use module federation to combine shared libraries (react, react-dom, axios). Is it problematic to use this? I am eagerly waiting for your answer. |
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. |
Summary
This PR was created to add support for Module Federation it is originated from this issue: #926
It aims to add a new option to the loadabel babel-plugin called
moduleFederation
, it's a boolean option so we just need to set it totrue
to enable the support for Module Federation.This PR also adds some JSDoc typedefs to the plugin, which will help users to understand the plugin better.
Test plan
To test it you can clone the sample project using this branch as base for the test
The branch with our samples fixed
https://github.com/brunos3d/loadable-module-federation-react-ssr/tree/demo-loadable-fix
The PR of the fixed branch to check the diff
brunos3d/loadable-module-federation-react-ssr#1