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

[runtime] Manifest fetch/logger error can throw unhandleable errors #3452

Open
5 tasks done
hrmcdonald opened this issue Jan 21, 2025 · 6 comments
Open
5 tasks done

Comments

@hrmcdonald
Copy link

hrmcdonald commented Jan 21, 2025

Describe the bug

Issue

When the MF runtime attempts to fetch any remote registered with an mf-manifest.json, if it fails, an exception is thrown. Today, there is no hook for handling this. A hook exists to provide the fetch itself, .catching that fetch does not resolve the issue here at all since other internal logic still "logs" the error leading to the exception being thrown regardless.

Depending upon what triggered the runtime to attempt to attempt the fetch, it might even not be something we can catch at all. In this recreation project, I am triggering the manifest fetch manually with a preloadRemote call. However, in our more complicated internal project (that I unfortunately cannot share here) even just shared dependency loading can lead to remote manifests getting fetched and throwing an error we cannot catch and handle. This happens in the snapshot plugin even if nothing is done with those remotes yet. This leads to the MF runtime throwing exceptions beyond our control that crash portions of our app like routing which can affect routes unrelated to the unreachable remote when any remote is unavailable at all for some reason.

Cause

Any call to SnapshotHandler.getManifest() will cause an unhandled exception to get thrown that we potentially have no way to handle. This can be triggered by preloadRemote, but also by the MF runtime itself when, for example, ShareHandler.initializeSharing() is called. That can trigger initRemoteModule() and eventually getManifest().

While the fetch within getManifest() is itself handled with a try/catch, it calls an error(msg) logging method that then rethrows an error with the logged message. I think this is really the root of the issue here tbh.

Why is this a problem? Well because often times routing can trigger dependencies to be imported. The error thrown here disrupts that process and causes at least @tanstack/react-router to fail on any route change even those not obviously related to the problematic remote in any way.

Links to referenced methods:

Potential Solution

We need a way to handle any errors thrown by the getManifest fetch before it calls error(msg) or really just exceptions thrown by error(msg) function in general. Anything that could log an error through that error method might result in this issue to be honest. (Does this need to throw the message instead of just log it as an error?)

Throwing errors are fine, as long as we have a path to hook into handling them. An errorLoadManifest hook might fit in line with errorLoadRemote? Alternatively just a handleError hook for dealing with errors that get logged via error(msg) instead of having that rethrow them no matter what.

Potential Alternatives

  • errorLoadRemote does not help here. This gets thrown separately from that hook regardless. I included the common example implementation for handling issues with that plugin hook to showcase it not helping here.
  • This is not a CORs issue. If a remote is unavailable for a moment, we can't have the entire site potentially throwing errors
  • We could potentially add some custom logic to error handling in our router more globally that tries to catch errors thrown up from dependency resolution it inadvertently triggers, but it'd be difficult to reliably discern what is a generic error being thrown by the MF runtime and how to handle it properly from there. Primarily I think the issue with this path is that in routers there isn't a way to tell it the error is OK to ignore. Only a way to handle what to render to inform the user it has happened.
  • A similar thread suggested a plugin that adjusts shareStrategy fixes this. It fixes a similar issue, but will not help handle this kind of exception getting thrown at all.

There are a few other past closed issues I read through on this that were either only tangentially related or never really uncovered the heart of this particular issue. If I did miss one that calls out a way to handle this kind of edge-case though please let me know and I'll review to confirm what fix/workaround/implementation adjustment works.

Reproduction

https://github.com/hrmcdonald/mf-runtime-fetch-manifest-issue

Used Package Manager

npm

System Info

System:
    OS: macOS 14.7.2
    CPU: (12) arm64 Apple M2 Max
    Memory: 85.31 MB / 32.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 20.18.0 - ~/.nvm/versions/node/v20.18.0/bin/node
    npm: 10.8.2 - ~/.nvm/versions/node/v20.18.0/bin/npm
    pnpm: 8.15.9 - ~/.nvm/versions/node/v20.18.0/bin/pnpm
  Browsers:
    Chrome: 132.0.6834.84
    Safari: 18.2

Validations

@danpeen
Copy link
Contributor

danpeen commented Jan 23, 2025

let me check

@DGAISmith
Copy link

I believe we have just hit this as well. Our Nx monorepo loads remotes at startup via init(), but if any remote is misconfigured / unavailable then we hit getManifest(), which throws, and there doesn't seem to be a way to catch the error so our whole app crashes.

@danpeen
Copy link
Contributor

danpeen commented Feb 5, 2025

i think we should let the errorLoadRemore hook catch this error. and after that we could do something to prevent this error crashes the whole app.

@hrmcdonald
Copy link
Author

i think we should let the errorLoadRemore hook catch this error. and after that we could do something to prevent this error crashes the whole app.

Yes, that makes total sense if that is the path y'all want to take. I just didn't want to be presumptive in the original issue. Just having some way to handle errors thrown here would be great.

@danpeen
Copy link
Contributor

danpeen commented Feb 7, 2025

Hi @hrmcdonald and @DGAISmith,

I'm pleased to inform you that we've merged the PR addressing this issue. We've also published a comprehensive guide on handling remote rendering errors, which you can find here:
https://module-federation.io/blog/error-load-remote.html

We're planning to release an official version today that includes these improvements. The blog post details various strategies for handling remote rendering failure scenarios, which should help address the challenges you've encountered.

Once the official release is out, you'll be able to implement these error handling strategies in your application. I encourage you to try them out and let us know how they work for you.

Your feedback and suggestions are always valuable to us as we continue to improve Module Federation. Please don't hesitate to share your thoughts or reach out if you need any assistance.

Best regards.

@hrmcdonald
Copy link
Author

Awesome news, the new blog post is great to see as well! We'll take a look when this update is published. Thanks again for looking into and resolving this one so quickly 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants