-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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(module-federation): add NxModuleFederationPlugin for inferred usage #30003
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
View your CI Pipeline Execution ↗ for commit 6203d88.
☁️ Nx Cloud last updated this comment at |
4d7a22f
to
0174987
Compare
// in addition to writing into the stdout stream, also show error directly in console | ||
// so the error is easily discoverable. 'ERROR in' is the key word to search in webpack output. | ||
if (stdoutString.includes('ERROR in')) { | ||
console.log(stdoutString); |
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.
should this be logged via console.error
?
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.
This was copied from the existing build-static-remotes
function that we have for executor usage. I think this is fine for now.
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.
Do you not think it is a better DX for errors to be logged via console.error
?
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.
Could be a good opportunity to change it 🤔 @jaysoo you originally added this logic for handling errors - is there a reason you used console.log and not console.error?
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.
It would be great to add some unit / e2e tests to be sure of its core functionality and prevent regressions down the line.
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.
Approved as to not block it from being merged
0174987
to
6203d88
Compare
Current Behaviour
Currently, Module Federation with Nx is forced to use executors to provide the best DX.
Expected Behaviour
As part of the transition to inferred targets, we will need Rspack plugins that replicates the DX provided by our executors.
Add
NxModuleFederationPlugin
andNxModuleFederationDevServerPlugin
to handle this.