Skip to content

feat(browser): Add lazyLoadIntegration utility #11339

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

Merged
merged 5 commits into from
Apr 8, 2024
Merged

Conversation

mydea
Copy link
Member

@mydea mydea commented Mar 28, 2024

This can be used to lazy load a pluggable integration.

Usage:

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

@mydea mydea requested review from billyvg, ryan953 and c298lee March 28, 2024 14:48
@mydea mydea self-assigned this Mar 28, 2024
Copy link
Contributor

github-actions bot commented Mar 28, 2024

size-limit report 📦

Path Size
@sentry/browser (incl. Tracing, Replay, Feedback) 75.65 KB (0%)
@sentry/browser (incl. Tracing, Replay) 66.87 KB (0%)
@sentry/browser (incl. Tracing, Replay with Canvas) 70.73 KB (0%)
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 60.44 KB (0%)
@sentry/browser (incl. Tracing) 31.75 KB (0%)
@sentry/browser (incl. browserTracingIntegration) 31.75 KB (0%)
@sentry/browser (incl. feedbackIntegration) 30.88 KB (0%)
@sentry/browser (incl. feedbackModalIntegration) 30.9 KB (0%)
@sentry/browser (incl. feedbackScreenshotIntegration) 30.9 KB (0%)
@sentry/browser (incl. sendFeedback) 26.89 KB (0%)
@sentry/browser 22.1 KB (0%)
CDN Bundle (incl. Tracing, Replay, Feedback) 71.63 KB (+0.51% 🔺)
CDN Bundle (incl. Tracing, Replay) 66.38 KB (+0.53% 🔺)
CDN Bundle (incl. Tracing) 32.7 KB (+1.14% 🔺)
CDN Bundle 24.13 KB (+1.57% 🔺)
CDN Bundle (incl. Tracing, Replay) - uncompressed 206.99 KB (+0.59% 🔺)
CDN Bundle (incl. Tracing) - uncompressed 97.59 KB (+1.24% 🔺)
CDN Bundle - uncompressed 71.37 KB (+1.71% 🔺)
@sentry/react (incl. Tracing, Replay) 66.99 KB (0%)
@sentry/react 22.12 KB (0%)

return existing;
}

const url = `https://browser.sentry-cdn.com/${SDK_VERSION}/${bundle}.min.js`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this domain configurable (now or future). It would also help with local dev to point to localhost for example.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a cdnBaseUrl config option for the client!

@getsentry getsentry deleted a comment from codecov bot Mar 28, 2024
@AbhiPrasad
Copy link
Member

For ESM builds, should we expose a utility that uses await import under the hood?

If someone uses bundlers it should code split automatically. I fear this will be less effective in certain scenarios because adblockers will block https://browser.sentry-cdn.com, not an issue for bundled code.

@mydea mydea force-pushed the fn/lazy-load-integration branch from 6aa9ec2 to dee0033 Compare April 2, 2024 10:15
@mydea
Copy link
Member Author

mydea commented Apr 2, 2024

For ESM builds, should we expose a utility that uses await import under the hood?

If someone uses bundlers it should code split automatically. I fear this will be less effective in certain scenarios because adblockers will block https://browser.sentry-cdn.com, not an issue for bundled code.

Hmm not sure, wouldn't it be better for them to just await import the integration themselves then? If you have a bundler there is no need really to use this if you don't want to...?

@mydea mydea force-pushed the fn/lazy-load-integration branch 2 times, most recently from aac25b7 to 2efd866 Compare April 4, 2024 07:16
@mydea mydea changed the title feat(browser): add lazyLoadIntegration utility feat(browser): Add lazyLoadIntegration utility Apr 4, 2024
Copy link
Member

@ryan953 ryan953 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets merge this in so we can start testing/running it end to end! Overall it's looking ready to me.

The LazyLoadableIntegrations list is something to maintain going forward, but since the integration and bundle names are different seems like an inherent the cost of doing this kind of thing.

@mydea mydea force-pushed the fn/lazy-load-integration branch from f2eb5e4 to 12eecd7 Compare April 8, 2024 08:09
@mydea mydea merged commit 9c8f0d3 into develop Apr 8, 2024
@mydea mydea deleted the fn/lazy-load-integration branch April 8, 2024 11:10
cadesalaberry pushed a commit to cadesalaberry/sentry-javascript that referenced this pull request 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 this pull request may close these issues.

Lazy Loading Integrations
4 participants