When using the webpack plugin, if the beforeRequest hook throw an error, the errorLoadRemote hook is never called

[patricklafrance]

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.

[ScriptedAlchemy]

@2heal1 @zhoushaw

should error load remote include lifecycle param as well?

loadRemoteArgs = await this.hooks.lifecycle.errorLoadRemote.emit({lifecycle: 'beforeRequest', error, origin: this, args})

[ScriptedAlchemy]

@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?

[patricklafrance]

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.

[ScriptedAlchemy]

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

[patricklafrance]

Thank you @ScriptedAlchemy 🙏

Is there a release including this fix?

[ScriptedAlchemy]

yes already released

[patricklafrance]

Awesome i’ll give it a try, thank you 🙏

[patricklafrance]

@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".

[ScriptedAlchemy]

Seems to work in the manifest-demo app in this repo.

[patricklafrance]

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.

[ScriptedAlchemy]

Okay when checking your installed source, i dont see my patch so im guessing the source code is not released for some reason

[ScriptedAlchemy]

@patricklafrance try with this release. https://github.com/module-federation/core/actions/runs/8949225786/job/24583264969

[patricklafrance]

Indeed it does work with 0.0.0-next-20240504082822!

[patricklafrance]

@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.

[ScriptedAlchemy]

Maybe consider sending a pull request and you can adjust the mechanics as you would like them to be.

[patricklafrance]

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?

[ScriptedAlchemy]

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

[patricklafrance]

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 👍🏻