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

Lazy Loading Integrations #10905

Closed
mydea opened this issue Mar 4, 2024 · 3 comments · Fixed by #11339
Closed

Lazy Loading Integrations #10905

mydea opened this issue Mar 4, 2024 · 3 comments · Fixed by #11339
Assignees

Comments

@mydea
Copy link
Member

mydea commented Mar 4, 2024

This is especially a requirement for feedback & screenshot capturing, but ideally we can build this in a generic way so also others things (e.g. the Loader) can leverage it.

For feedback, we want to allow users to capture & annotate screenshots. This is quite some code, so we want to lazy load this. However, it is not super easy today to set up lazy loading, and it requires bundler-specifics to work properly.

This proposes to provide an easy to use solution for this: client.lazyAddIntegration(integrationName).

This API could be used like this:

// instead of
const { feedbackIntegration } = await import('@sentry/browser');
getClient().addIntegration(feedbackIntegration(feedbackOptions));

// You can do:
getClient().lazyAddIntegration('feedbackIntegration', feedbackOptions);

Which would under the hood do something like this:

async function lazyAddIntegration(functionName: string, options: any) {
  const fileName = convertFunctionNameToFileName(functionName); // TODO, maybe an allow-list?
  const url = `https://browser.sentry-cdn.com/${SDK_VERSION}/${fileName}.min.js`;
  const script = document.createElement('script');
  script.setAttribute('src', url);
  
  const waitForLoad = new Promise((resolve, reject) => {
    script.addEventListener('load', resolve);
    script.addEventListener('error', reject);
  });

  document.body.appendChild(script);
  await waitForLoad;

  const integration = typeof window[functionName] === 'function' ? window[functionName](options) : undefined;
  if(integration) {
    getClient().addIntegration(integration);
  }
}

This leverages the fact that we can already publish integrations as CDN bundles, which are versioned through the URL. This way, we can easily load the correct version of an integration for the current version.

The additional benefit of this is that it fully interops with non-lazy loading. E.g. if you don't want this, you can just do addIntegration() normally - nothing needs to be done! This also means that testing this is easy, and users who may not want to load from a CDN can just await import() themselves as before.

@timfish
Copy link
Collaborator

timfish commented Mar 5, 2024

Can we not just use await import() in the integration and then it'll work for users who aren't using the CDN. Then when we bundle the CDN integration bundles, the integration will automatically split into two files by our bundler?

@mydea
Copy link
Member Author

mydea commented Mar 6, 2024

The problem with this is mainly that the fact if this works depends on the users of the SDK having a bundler that supports await import() and handles it properly - it may or may not work for users, I believe?
Also for the CDN bundles I don't think this will work automatically too, because this goes to a URL etc. so would need an import map (I guess?) or something like this...?

@timfish
Copy link
Collaborator

timfish commented Mar 6, 2024

The problem with this is mainly that the fact if this works depends on the users of the SDK having a bundler that supports await import() and handles it properly - it may or may not work for users, I believe?

The only bundler I can remember thats lacking here is esbuild and even then, worse case it seems to inline the import so you'd just lose the laziness. There might be some others though! Or as you suggest, a non-lazy version as a backup?

Also for the CDN bundles I don't think this will work automatically too, because this goes to a URL etc. so would need an import map (I guess?) or something like this...?

In the browser, import and async import both just use URLs, whether they are relative or absolute they're still just all URLs. The only caveat is that the JavaScript files must be served with the correct mime type and cors headers if it's cross origin.

Import maps just allow developers to replace the URLs with some string names.

For example Sentry could publish bare browser ESM files that can be used like this in any modern browser:

<script type="module">
  import { init } from 'https://browser.sentry-cdn.com/7.105.0/esm.js';

  init({ dsn: '__DSN__' });

  const integration = await import('https://browser.sentry-cdn.com/7.105.0/some-integration.js');
</script>

mydea added a commit that referenced this issue Apr 8, 2024
This can be used to lazy load a pluggable integration.

Usage:

```js
async function getOrLazyLoadFeedback() {
  const existing = Sentry.getFeedback(); // check if it has already been installed
  if (existing) {
    return existing;
  }
  try {
    const feedbackIntegration = await Sentry.lazyLoadIntegration('feedbackIntegration');
    client.addIntegration(feedbackIntegration());
  } catch(error) {
    // this can error, we need to handle this!
  }
}
```

Closes #10905
cadesalaberry pushed a commit to cadesalaberry/sentry-javascript that referenced this issue Apr 19, 2024
This can be used to lazy load a pluggable integration.

Usage:

```js
async function getOrLazyLoadFeedback() {
  const existing = Sentry.getFeedback(); // check if it has already been installed
  if (existing) {
    return existing;
  }
  try {
    const feedbackIntegration = await Sentry.lazyLoadIntegration('feedbackIntegration');
    client.addIntegration(feedbackIntegration());
  } catch(error) {
    // this can error, we need to handle this!
  }
}
```

Closes getsentry#10905
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

Successfully merging a pull request may close this issue.

2 participants