Skip to content

feat(fedback): Convert CDN bundles to use async feedback for lower bundle sizes #11791

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.

Sign up for GitHub

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 8 commits into from
Apr 30, 2024
Merged

feat(fedback): Convert CDN bundles to use async feedback for lower bundle sizes #11791

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
69657ad
feat(fedback): Convert CDN bundles to use async feedback by default
ryan953 Apr 25, 2024
f471401
export feedbackAsyncIntegration in bundles
ryan953 Apr 25, 2024
318e480
export feedbackAsyncIntegration in all the bundles, as well as feedba…
ryan953 Apr 29, 2024
c7f2e4a
Merge branch 'develop' into ryan953/feedback-async-default
ryan953 Apr 29, 2024
d8994b2
linty
ryan953 Apr 29, 2024
b4188c1
fix line breaks
ryan953 Apr 30, 2024
f32acf4
linting
ryan953 Apr 30, 2024
1e27ac4
linting
ryan953 Apr 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/browser/src/index.bundle.feedback.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Copy link
Member Author

Choose a reason for hiding this comment

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

@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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

So there are two aspects to this:

  1. 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 for getIntegrationByName('Feedback') would return either one of these if installed (benefit of this is that it would become impossible to install both).
  2. 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:

  1. Is the API the same for both? I guess yes, then all good, but if not it may become confusing.
  2. 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 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

export { getFeedback } from '@sentry-internal/feedback';

export { browserTracingIntegrationShim as browserTracingIntegration, replayIntegrationShim as replayIntegration };
2 changes: 1 addition & 1 deletion packages/browser/src/index.bundle.tracing.replay.feedback.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ export {
setMeasurement,
} from '@sentry/core';

export { feedbackIntegration } from './feedback';
export { feedbackAsyncIntegration as feedbackIntegration } from './feedbackAsync';
export { getFeedback } from '@sentry-internal/feedback';

export {
Expand Down