feat(fedback): Convert CDN bundles to use async feedback for lower bundle sizes #11791
Conversation
size-limit report 📦
|
Love to see this! 12% of the bundle size was from loading the feedback modules which only render after the user clicks the button. For most pageviews people will not click the button, so thats a large number of bytes saved per pageload. |
| @@ -2,7 +2,7 @@ import { browserTracingIntegrationShim, replayIntegrationShim } from '@sentry-in | |||
|
|
|||
| export * from './index.bundle.base'; | |||
|
|
|||
| export { feedbackIntegration } from './feedback'; | |||
| export { feedbackAsyncIntegration as feedbackIntegration } from './feedbackAsync'; | |||
There was a problem hiding this comment.
@mydea What do you think about this alias?
The idea is that the docs will use feedbackAsyncIntegration for our npm setup instructions. So that's what people will copy paste and see all over the place.
But for the CDN, most people probably won't interact with the integration name right?
I wanted to have a stable name so that we can flip from async to sync in the future if we wanted to and have it be transparent.
The CDN could alternatively ship feedbackAsyncIntegration to start, and then in the future ship both feedbackAsyncIntegration and feedbackIntegration.
There was a problem hiding this comment.
Actually, i'm thinking now that things like getIntegrationByName would still need to have 'feedbackAsyncIntegration' passed in because that's what the instance will be called. so the alias here isn't adding confusion to that case.
There was a problem hiding this comment.
So there are two aspects to this:
- Integration Name: this is what is used for e.g.
getIntegrationByName, but this is e.g.Feedback, not the function name. Technically both the async and sync integration could have the same name, meaning any check forgetIntegrationByName('Feedback')would return either one of these if installed (benefit of this is that it would become impossible to install both). - The function name: This will have to be used even on the CDN, as users still need to add it to their init. So it's def. a difference as what we export this.
Usage would be:
<script src="URL_TO_CDN_WITH_FEEDBACK"></script>
<script>
Sentry.init({
integrations: [Sentry.feedbackAsyncIntegration()]
});
</script>I am not so sure, it feels a bit weird to expose this to users like this 🤔 When using the CDN, is there ever a reason to not use the async variant? E.g. the only reason to do that in NPM is to avoid loading stuff from the CDN, which you are already doing anyhow in the CDN scenario 😅
So I think I would tend to just expose this as feedbackIntegration(). But this of course has two potential issues:
- Is the API the same for both? I guess yes, then all good, but if not it may become confusing.
- It may become confusing on NPM if we want to expose this as separate integrations, because suddenly they are not the same in NPM/CDN land.
I think part of the issue is that overall the naming of feedbackAsyncIntegration is not very user-friendly and clear (e.g. what does async mean in this context, ...). Mind you, I don't have any better ideas either 😅
There was a problem hiding this comment.
100%... I had some feeling about some of these points but the getIntegrationName i was less sure about. You laid it out well, let see what levers we can pull to improve it:
getIntegrationName
This should be Feedback in both cases, because buildFeedbackIntegration returns the same object, with different dependencies injected. So this sounds like the ideal ✅
CDN default impl and function name
For the CDN bundle, we'll go with the slim bundle size, and async loading strategy. So this PR will deliver that.
What's the function name though?
Some ideas:
// basic name, no hint about strategy:
integrations: [Sentry.feedbackIntegration()]
// strategy specific name, can help with clarifying the docs
integrations: [Sentry.feedbackAsyncIntegration()]
// different strategy specific name:
integrations: [Sentry.lazyFeedbackIntegration()]
// different strategy specific name (not as good):
integrations: [Sentry.feedbackOnInteractionIntegration()]I think we should write the docs and that could help a lot
There was a problem hiding this comment.
What about this:
We always (CDN & NPM) have a feedbackIntegration, which uses some undefined loading strategy. Basically we'll use what we think works better, which on CDN is async and on NPM is sync.
Then in addition we also have feedbackAsyncIntegration and feedbackSyncIntegration. On CDN only the former exists, on NPM both exist.
Does that make sense?
There was a problem hiding this comment.
yes! i like this. It's good for docs-writing, because we can be consistent everywhere, and we'll just add the section for loading strategies to disambiguate. In that section can explain how it's Async for all the web-facing stuff, and we can make the electron package be Sync to both prove the point, but that's what I'd expect there anyway.
I'll update this PR in a sec
packages/browser/src/index.ts
Outdated
| export { | ||
| feedbackAsyncIntegration, | ||
| feedbackSyncIntegration, | ||
| feedbackSyncIntegration as feedbackIntegration | ||
| }; |
There was a problem hiding this comment.
NPM users will have both strategies to pick from, but feedbackIntegration is the name we have all over the docs, so that's available too using the Sync strat.
|
|
||
| import * as ReplayBundle from '../../src/index.bundle.replay'; | ||
|
|
||
| describe('index.bundle.replay', () => { | ||
| it('has correct exports', () => { | ||
| expect(ReplayBundle.replayIntegration).toBe(replayIntegration); | ||
| expect(ReplayBundle.browserTracingIntegration).toBe(browserTracingIntegrationShim); |
There was a problem hiding this comment.
Some of the test files were missing various assertions (none of them assert that getFeedback or the long list of tracing helpers exist)
I made sure they all assert that browserTracingIntegration is set, and the order of assertions is consistent to hopefully make it easier going forward to verify what's here and add new ones.
It's more aggressive to start with the async loading strategy in the CDN bundle, but could be safer because if we want to change from async->sync it would not really be a majr version change. Whereas going from sync->async would be more of a big feature, riskier, and could demand a version bump or something.