Lazy load feedback screenshot

[c298lee]

From Francesco:
So, just writing down my thoughts on how we could lazy load the feedback screenshot stuff:

1. We package the code into a new integration, similar to @sentry-internal/replay-canvas, e.g. @sentry-internal/feedback-media or something like this.
2. This can be added in addition to Feedback to enable this. Users can add this manually, either lazy loading it themselves or just eagerly including this - normal integration stuff!
3. If users do not add this, what we'll do if a user clicks "Add screenshot" is inject a <script> tag to the integration CDN bundle into the page, and go from there. e.g. something like this:

function lazyLoadFeedbackMedia() {
  const tag = `<script src='[https://cdn.sentry.com/${SDK_VERSION}/integration.feedback-media.min.js](https://cdn.sentry.com/$%7BSDK_VERSION%7D/integration.feedback-media.min.js)'></script>';
  // inject tag into page
  tag.onLoad = function() {
    // continue when everything is ready
  }
}

[ryan953]

So this is the full proposal for lazy loading within the Feedback portion of the SDK.

For reference we're leveraging:
#11339 & #10905

And existing PRs you might've seen for the first few steps are:
#11342
#11355

---

Why do we want async loading?

There's an understanding that the feedback feature is a bit heavy bytes wise, and will get bigger as more features are added over time. So we want to code-split unused bytes.

The goal for our new async loaded Feedback is to have an SDK that supports these cases/combinations (in no specific order):

1. "Async nothing" would allow for bundling the button, form, and screenshot input.
2. "Async screenshot" would allow for bundling the "send feedback button AND the form, async load the screenshot input
3. "Async after interaction" would allow for bundling the "send feedback" button, async load the form and screenshot input
4. "Async everything"

   I don't think we should do this, but for completeness this is something we could do. Users could async load everything: the button, form and screenshot input by calling lazyLoadIntegration themselves. The problem with this is that the button needs to render immediately, so async loading it won't be a great user experience. It could look something like this:

   import * as Sentry from "@sentry/browser";
   Sentry.init({
     dsn: "foo",
     integrations: [Sentry.replayIntegration()],
   });
   const feedbackIntegration = await Sentry.lazyLoadIntegration('feedbackIntegration');
   
   Sentry.getClient()?.addIntegration(feedbackIntegration({
   
   colorScheme: "light",
   
   }));

I've made some assumptions about where the lines should be draw for code-splitting and async loading based on what code is needed, and how often. Assumption being that the button is shown very often, but clicks happen less often, so we should split at that boundary. Including the screenshot feature is optional as well (not supported on mobile) so we should also split that up.

todo: check click rates on the existing 'send feedback' button

JS SDK v7 API

In the v7 JS SDK we have an api that looks like this:

import * as Sentry from "@sentry/browser";
Sentry.init({
  dsn: "foo",
  integrations: [
    Sentry.feedbackIntegration({
      colorScheme: "light",
    }),
  ],
});

This bundles together the Button and Form, nothing is async loaded.

v8 Proposal

The end-goal for v8 is that we have two options for users to choose from. Which snippet is the default is left for another conversation, not something we need to decide here and now.

The "Async Nothing" snippet (everything is bundled together). This is the exact same snippet as the v7 api, with the addition of Screenshots + cropping + annotations and anything else also bundled in.

import * as Sentry from "@sentry/browser";
Sentry.init({
  dsn,
  integrations: [
    Sentry.feedbackIntegration({
      colorScheme: "light",
      showScreenshot: true,
    }),
  ],
});

The "Async after interaction" snippet. The difference here is the name change from feedbackIntegration to feedbackAsyncIntegration (or whatever name we choose).

import * as Sentry from "@sentry/browser";
Sentry.init({
  dsn,
  integrations: [
    Sentry.feedbackAsyncIntegration({
      colorScheme: "light",
      showScreenshot: true,
    }),
  ],
});

Alternate v8 API design ideas

So far there are few alternate ideas that make it possible to async load the different parts outlined above.

If we have any `import` statement inside `feedbackIntegration` then un-bundling that code becomes really hard, so that's why the proposal above suggested two integration names for users to choose from (we will have a default snippet that is one of the two options, so users don't need to stress as much).

Therefore, if we never wanted to allow async loading of the Form, then we could not implement feedbackAsyncIntegration and maybe do a few other things differently. I think it's premature to make this decision though. What we need to make this decision is bundle size metrics and metrics on how many people see the button vs. clicking on the button.

Steps to get to the stated end-goal

If we agree on that end-goal for the api, the steps to get there are well defined, as of 8.0.0-alpha.7:

1. We need to make v8 API compatible with v7 to unblock any 8.0.0-beta releases
   We can do this with a patch to packages/feedback/core/integration.ts, something like:

     + import { feedbackModalIntegration } from '@sentry-internal/feedback-modal';
     ...
     export const feedbackIntegration = (({
         ...
     -   const modalIntegration = client.getIntegrationByName('FeedbackModal');
     +   const modalIntegration = feedbackModalIntegration({});
     +   client.addIntegration(modalIntegration);
         ...
     -   } else if (!modalIntegration) {
     -   throw new Error('Async loading of Feedback Modal is not yet implemented');

2. We need to split up existing packages/feedback folder to better isolate and control imports. Such that core/ & modal/ and screenshot/ don't create circular imports, and don't import and re-bundle the same helpers/utils/core files.
   After this step we should be able to get proper bundle size measurements to confirm that this is the right division of code, and worth the build-time overhead.
   PRs to expect

   • https://github.com/ref(feedback): Create stub integrations for feedback modal & screenshot integrations #11342 aims to setup the skeleton for that separation
   • https://github.com/ref(feedback): Refactor Feedback types into @sentry/types and reduce the exported surface area #11355 deals with the type imports that would be shared
   • a 3rd diff would move actual implementation files
3. Change feedbackIntergration to be implemented in terms of feedbackAsyncIntegration
   feedbackAsyncIntegration would have some useful options

   This is the async integration, with default values null meaning to async load things:
   

   
   
   integrations: [
   
   feedbackAsyncIntegration({
   
   showScreenshot: true,
   
   getModalIntegration: null,
   
   getScreenshotIntegration: null,
   
   }),
   
   ]

   Therefore feedbackIntergration would import & inject the other two integrations. Users would not need to import from any @sentry-internal/* package.
   

   
   
   import {feedbackModalIntegration} from '@sentry-internal/feedbackModal';
   
   import {feedbackScreenshotIntegration} from '@sentry-internal/feedbackScreenshot';
   
   return feedbackAsyncIntegration({
   
   showScreenshot: true,
   
   getModalIntegration: feedbackModalIntegration,
   
   getScreenshotIntegration: feedbackScreenshotIntegration,
   
   });

[Lms24]

Thanks for outlining the plan, Ryan! I like the v8 proposal - two (from user perspective) very simple integrations to choose from!

We need to make v8 API compatible with v7 to unblock any 8.0.0-beta releases

Maybe I don't understand the intention here correctly but semver-wise it's fine to break things as we're still in a v8 alpha stage. Just once we go into v8 beta, we shouldn't add breaking changes anymore. But adding features (or new integrations) is totally fine!

[mydea]

That sounds good to me as well! Ideally we can have a feedbackIntegration that does everything ASAP (then we can cut a beta release with this), we can then do further stuff while the beta is ongoing? It would be nice if we cut cut the beta monday or latest tuesday next week IMHO, does that sound achievable? :)

[ryan953]

@Lms24 & @mydea #11461 should be enough to make the v8 branch compatible with v7, it's draft right now because it's going to create some minor merge conflicts and i just want to get a clean build running locally first.

We did cut https://github.com/getsentry/sentry-javascript/releases/tag/8.0.0-alpha.9 a few hours ago, so if the CI is happy I will have this PR merged before we move from alpha to beta branches.