sentry-javascript icon indicating copy to clipboard operation
sentry-javascript copied to clipboard

TypeError: Sentry.moduleMetadataIntegration is not a function when using loader script

Open gilisho opened this issue 1 year ago • 11 comments

Is there an existing issue for this?

  • [x] I have checked for existing issues https://github.com/getsentry/sentry-javascript/issues
  • [x] I have reviewed the documentation https://docs.sentry.io/
  • [x] I am using the latest SDK release https://github.com/getsentry/sentry-javascript/releases

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/browser

SDK Version

7.119.0

Framework Version

No response

Link to Sentry event

No response

Reproduction Example/SDK Setup

Similarly to #13297, I am using a loader script and moduleMetadataIntegration seems to be unavailable when using loader script. I need this integration as advised for micro-frontend environment, and to use that solution I am migrating from Sentry SDK 5.30.0 to 7.119.0, which is already challenging.

Any reason to not have a dedicated bundle for this integration like other integration have? This is a necessity for micro-frontend environment so I really hope there's a way to work around it .

Would appreciate your help in the matter. 🙏

Steps to Reproduce

Use loader script for 7.x version. Have this script above:

<script>
  // Configure sentryOnLoad before adding the Loader Script
  // https://docs.sentry.io/platforms/javascript/install/loader/#custom-configuration
  window.sentryOnLoad = function () {
    console.log('sentryOnLoad')
    console.log(window.Sentry)
    window.Sentry.init({
      // moduleMetadataIntegration is a required integration for having the module_metadata available in runtime
      integrations: [window.Sentry.moduleMetadataIntegration()],
      // Solution for getting the route to the unhandled error.
      // This is the recommended solution for micro-frontend environment.
      beforeSend: function(event){
        if (event?.exception?.values?.[0].stacktrace.frames) {
          const frames = event.exception.values[0].stacktrace.frames;

            // Find the last frame with module metadata containing a DSN
          const framesModuleMetadata = frames
            .filter(
              (frame) => frame.module_metadata && frame.module_metadata.dsn,
            )
            .map((v) => v.module_metadata)

          const routeTo = framesModuleMetadata.slice(-1); // using top frame only - you may want to customize this according to your needs

          if (routeTo.length) {
            event.extra = {
              ...event.extra,
              ROUTE_TO: routeTo,
            };
          }
        }

        return event;
      }
    })
  }
</script>

Expected Result

Sentry is initialized successfully.

Actual Result

TypeError: Sentry.moduleMetadataIntegration is not a function

gilisho avatar Sep 25 '24 17:09 gilisho

@gilisho I don't think there is a specific reason for that, we should include this in our CDN so you can lazy-load it in you application. I'll put this on our backlog for now, PRs are also welcome of course!

For your application I'm afraid the only option right now is to use the integration via npm installation of our SDK.

chargome avatar Sep 26 '24 09:09 chargome

Hi @chargome ! I am very open to the idea of contributing a PR! Moreover, I will need it to be available in Major 7. Should I open two PRs for both major 7 and major 8? can you guide me a bit on the process? Also, I assume the PR should be pretty similar to #13241?

gilisho avatar Sep 26 '24 11:09 gilisho

@gilisho nice!

For general contributing guidelines check https://github.com/getsentry/sentry-javascript/blob/develop/CONTRIBUTING.md

So I think the FeedbackIntegration and ReplayIntegration are more complex cases but you could check for example any of these: https://docs.sentry.io/platforms/javascript/configuration/integrations/#2-load-from-cdn-with-lazyloadintegration for reference.

@Lms24 can you maybe chime in for handling v7 updates?

chargome avatar Sep 26 '24 12:09 chargome

It seems that I need to make changes and create a PR against v7 branch as well as develop, right? opened a draft PR against development #13817

gilisho avatar Sep 26 '24 19:09 gilisho

@gilisho yes once a pr on v7 is merged we can trigger a release

chargome avatar Sep 26 '24 22:09 chargome

I made 2 PRs for both v8 and v7. LMK if something is missing. This is the first-time I'm diving into Sentry's source code so I hope I'm not missing anything. 🙏

v8 PR - #13817 v7 PR - #13822

Since my platform is using v7 ATM, #13822 is in a higher priority for me. I'd like to know when this can get released to a new version. Waiting for this to get released. 🙏
Thank you!

gilisho avatar Sep 27 '24 11:09 gilisho

@gilisho thank you! We'll take a look 👍

chargome avatar Sep 30 '24 07:09 chargome

Thank you! I added the test you requested for #13817. LMK if something else is needed for both PRs.

Some concerns have been raised though:

  1. Checking with the bundle I produced locally for moduleMetadataIntegration, I can see that the module metadata is added to event.extra as I need, which is great. But, it only works when I use the unminified bundle. When I use the minified version, the module metadata is not added to the stacktrace frames. I've seen these kind of issues are possible, like #5527.

  2. Actually, I just realized it's not the only obstacle for the micro-frontend solution to work for my platform. The event is not being transferred to the right DSN because I'm having a similar issue now with TypeError: window.Sentry.makeMultiplexedTransport is not a function. Any suggestions? Could this also be included in the loader script alongside the other included transport utils like “makeFetchTransport”? I believe this would require a small change to export it here. Otherwise there's no way to use the frontend solution with loader script, and we use loader script since we want to lazy load.

gilisho avatar Oct 03 '24 12:10 gilisho

Regarding 2., makeMultiplexedTransport is not exported in the CDN bundle (which is also used by the loader), due to bundle size reasons. From my current POV, we are unlikely to add this to the CDN bundle because this will increase the bundle size for all consumers of this, even though the vast majority does not need it.

What I would do, is inline the transport code from https://github.com/getsentry/sentry-javascript/blob/0d42ae82022ca49a7471681e3c72c023a74b615f/packages/core/src/transports/multiplexed.ts#L89 into your application code and just use it from there.

mydea avatar Oct 07 '24 07:10 mydea

Alright. There's still a need to shed light on (1). It will be easier to do so once #13822 is merged. 😊 Right now I am using the un-minified bundle locally and it's not very ideal...

gilisho avatar Oct 08 '24 08:10 gilisho

  1. is def. still open, let's get the PR merged an then we can continue to investigate this!

mydea avatar Oct 08 '24 14:10 mydea

Hi @mydea, could you take a look again at #13822?

gilisho avatar Oct 29 '24 13:10 gilisho

Hi, I see both PRs against v8 and v7 are still open. Let's try to get the v8 one in first and then take a look at v7. Sorry that this is taking a while but we're a bit busy and when it comes to adding build/release artifacts we need to be extra careful.

Lms24 avatar Oct 30 '24 11:10 Lms24

That's ok. I'm still waiting for a review on v8 PR - #13817. v7 PR is also ready for review - #13822.

gilisho avatar Nov 10 '24 08:11 gilisho

Hi, before we merge these PRs, would you mind explaining your use-case in a bit more detail?

Personally, something feels off about having microfrontends + the loader. We need to be a bit more gatekeep-y with what we add to the loader because of bundlesize.

Btw I am guessing you would also need the multiplexed transport?

lforst avatar Nov 11 '24 08:11 lforst

Hi @lforst, thanks for the response!

would you mind explaining your use-case in a bit more detail?

I have one Sentry instance which is initialized using the Loader script in the host. The microfrontends are rendered inside this host and we use the micro-frontend solution for the host Sentry instance. I understand you do not want to add this integration to the loader's bundle. This is not what I am aiming for. All I want is to make sure moduleMetadataIntegration has a dedicated bundle that I can load lazily (or in v7 - not-lazily) in addition to the loader's script, just like it's possible for other integrations.

Btw I am guessing you would also need the multiplexed transport?

Yes, I have raised this concern earlier. @mydea already addressed that here and I am okay with the solution he proposed.

gilisho avatar Nov 11 '24 08:11 gilisho

@gilisho that makes sense. That is actually a valid use-case. Thanks for explaining! Let's get it merged then.

lforst avatar Nov 11 '24 08:11 lforst

@mydea Hey, unfortunately I do not have permission to reopen this issue. This issue is actually dependent on both PRs #13817 and #13822. V8 PR (#13817) is merged now which is super awesome! 🥳 Can we merge #13822 as well for the same support on v7? That would be great! (we are using v7 ATM)

gilisho avatar Nov 11 '24 09:11 gilisho

I merged both PRs, thank you for opening them! We'll include these in the next releases.

mydea avatar Nov 11 '24 15:11 mydea

Thank you so much for all the help! Do you know what is the ETA of the next release for v7?

gilisho avatar Nov 12 '24 08:11 gilisho

We'll try to cut a release in the near future, can't give a concrete ETA but hopefully should not be too long!

mydea avatar Nov 12 '24 09:11 mydea

A PR closing this issue has just been released 🚀

This issue was referenced by PR #13817, which was included in the 8.38.0 release.

github-actions[bot] avatar Nov 12 '24 14:11 github-actions[bot]