core
core copied to clipboard
When using the webpack plugin, if the beforeRequest hook throw an error, the errorLoadRemote hook is never called
Describe the bug
When using the webpack plugin, if the beforeRequest
hook throw an error, the errorLoadRemote
hook is never called.
With the following runtime plugin:
export default function () {
return {
name: "custom-runtime-plugin",
async beforeRequest(args) {
console.log("*********beforeRequest", args);
throw new Error("Error thrown from the beforeRequest hook.");
return args;
},
async errorLoadRemote(args) {
console.log("*********errorLoadRemote", args);
}
}
}
Configured in the host application wepack.config.js
file:
new ModuleFederation.ModuleFederationPlugin({
runtimePlugins: [
require.resolve("./runtimePlugin.js")
]
})
The Error
thrown from the beforeRequest
hook is never handled by the errorLoadRemote
hook and it crashes the application:
Reproduction
https://github.com/patricklafrance/mf-enhanced-before-request-issue
Used Package Manager
pnpm
System Info
System:
OS: Windows 11 10.0.22631
CPU: (12) x64 Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
Memory: 11.32 GB / 31.70 GB
Binaries:
Node: 21.7.1 - C:\Program Files\nodejs\node.EXE
Yarn: 1.22.19 - C:\Program Files (x86)\Yarn\bin\yarn.CMD
npm: 10.5.0 - C:\Program Files\nodejs\npm.CMD
pnpm: 8.15.4 - ~\AppData\Roaming\npm\pnpm.CMD
Browsers:
Chrome: 124.0.6367.61
Edge: Chromium (123.0.2420.97)
Internet Explorer: 11.0.22621.1
Validations
- [X] Read the docs.
- [X] Read the common issues list.
- [X] Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
- [X] Make sure this is a Module federation issue and not a framework-specific issue.
- [X] The provided reproduction is a minimal reproducible example of the bug.
@2heal1 @zhoushaw
should error load remote include lifecycle param as well?
loadRemoteArgs = await this.hooks.lifecycle.errorLoadRemote.emit({lifecycle: 'beforeRequest', error, origin: this, args})
@patricklafrance beforeRequest is not necessarily errorLoadRemote - its what generates the args for loadRemote. Im not sure if catching it here with error load remote hook would be correct because it still needs to return its args to make the request to load remote. So depending on what hook is called the outputs would have to be different.
errorLoadRemote can also return a function thats another component or module. So if you tried to do that for beforeRequest it would blow up too.
Whats your use case of beforeRequest?
I see. Isn't it part of the workflow of loading a remote though, even if it's happening "before" ?
I guess what you are saying makes sense if you expect errorLoadRemote
to render a fallback UI when an error happens. Personally, when a remote fails to load, what I want is to ignore the failing remote and proceed with the other remotes. I don't want to stall my users because a remote is failing when 95% of my app is probably still available.
Anyway, if it's not errorLoadRemote
that catches the error, I would at least expect loadRemote.catch
to catch the beforeRequest
error?
Currently, my use case for this would be to verify if a remote is available before proceeding with createScript
. This is related to https://github.com/module-federation/universe/issues/2367. The assumption is that beforeRequest
is async and createScript
is sync, so implementing a custom timeout mechanism would be preferable in beforeRequest
rather than createScript
.
Honestly, the best place to implements a timeout mechanish would be when fetching the manifest file, but I cannot currently use manifest files for my remote entry points because of https://github.com/module-federation/universe/issues/2362.
https://github.com/module-federation/core/pull/2373/files Needs to be documented but its been merged.
errorLoadRemote hook now has args.lifecycle.
if lifecycle is beforeRequest, you must return args for beforeRequest from hook if lifecycle loadRemote, you return the args loadRemote would | return fallback factory function (what it did before)
this hook now fires for each hook so you need to be careful and ensure that you return the right things for each lifecycle that it catches
Thank you @ScriptedAlchemy 🙏
Is there a release including this fix?
yes already released
Awesome i’ll give it a try, thank you 🙏
@ScriptedAlchemy, when you mentionned that it's released, are you saying that it's released with @module-federation/enhanced
version 0.1.11
?
Whenever I throw an error in beforeRequest
, it's still crashing the app:
And the errorLoadRemote
hook is never called:
[webpack-dev-server] Server started: Hot Module Replacement enabled, Live Reloading enabled, Progress disabled, Overlay enabled.
react refresh:6 [HMR] Waiting for update signal from WDS...
index.js:3 ************* index.js
I tested with the following runtime plugin:
export default function () {
return {
name: "custom-plugin",
async beforeRequest(args) {
throw new Error("beforeRequest custom error");
return args;
},
async errorLoadRemote(args) {
console.log("*********errorLoadRemote", args, args.lifecycle);
return args;
}
}
}
I would have expected that errorLoadRemote
would be called with args.lifecycle === "beforeRequest"
.
Seems to work in the manifest-demo app in this repo.
Hmm, weird, my POC is available here if you want to take a look: https://github.com/patricklafrance/mf-enhanced-poc/tree/before_request_error. Make sure to use the before_request_error
branch.
Okay when checking your installed source, i dont see my patch so im guessing the source code is not released for some reason
@patricklafrance try with this release. https://github.com/module-federation/core/actions/runs/8949225786/job/24583264969
Indeed it does work with 0.0.0-next-20240504082822
!
@ScriptedAlchemy while the error is handled by the errorLoadRemote
hook, it's not handled by loadRemote.catch
thought. Shouldn't it also be handled by loadRemote.catch
? Most of the failures related to loading a remote seems to also be handled by loadRemote.catch
.
I am testing this behavior by commenting my errorLoadRemote
hook.
Maybe consider sending a pull request and you can adjust the mechanics as you would like them to be.
Since it's working as expected with the errorLoadRemote
hook, should I close this issue now or you'll close it once it's release?
yeah go for it. I would suggest sending a PR our way, it may be easir to send over code and we can adjust is as needed vs back and forth over this lol. But yes you can close
Will close for now and open another issue if needed.
Yeah sounds good, I was suggesting also integrating the error handling to loadRemote.catch
in case you wanted the API to remain consistent 👍🏻