feat(feedback): Create async bundles and code to resolve helper integrations #11621

ryan953 · 2024-04-16T04:24:37Z

This imports the various feedback bits into @sentry/browser and re-exports them as two 'built' integrations:

feedbackIntegration
feedbackAsyncIntegration

The idea is that the former includes everything already imported, and the latter will leverage lazyLoadIntegration to bring in extra code when needed. This means that Sentry users can decide which integration to import as they setup the SDK. We can decide to a favored approach when we write the docs and installation instructions, right now for npm/yarn/pnpm users both options are viable.

There is, as of this PR, no CDN or Loader bundle that leverages async loading. The CDN bundles are published with all the code included.

The bundle sizes, compressed, are currently at:

bundle name	additional size from prev row	code included
`@sentry/browser 21.67 KB`		base size
`@sentry/browser (incl. sendFeedback) 26.45 KB`	+4.78KB over the base	widget depends on this, see #11428
`@sentry/browser (incl. FeedbackAsync) 30.93 KB`	+4.48KB "incl. sendFeedback"	Actor button & async bootstrapping
`@sentry/browser (incl. Feedback) 37.78 KB`	+6.85KB over "incl. FeedbackAsync"	everything

The expectation is that as we add stuff to the Feedback bundle, FeedbackAsync won't get any larger at all.

To followup:

What should the developer see, and what should the user see if integrations are missing and unable to be loaded in?
It seems like preact is not tree-shaken out of the main async bundle. Need to double check this. Running yarn site-limit --why and looking at @sentry/browser (incl. FeedbackAsync) is what I was looking at
Some types are unknown in the @sentry/types package, but not used outside of @sentry/feedback. These can probably be re-integrated and improved in a followup PR.

Fixes #11435

…rations

github-actions · 2024-04-16T04:30:09Z

size-limit report 📦

Path	Size
@sentry/browser	21.67 KB (0%)
@sentry/browser (incl. Tracing)	31.4 KB (0%)
@sentry/browser (incl. Tracing, Replay)	66.73 KB (-0.01% 🔽)
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags	60.14 KB (-0.01% 🔽)
@sentry/browser (incl. Tracing, Replay with Canvas)	70.57 KB (-0.01% 🔽)
@sentry/browser (incl. Tracing, Replay, Feedback)	82.96 KB (+3.14% 🔺)
@sentry/browser (incl. Feedback)	37.78 KB (+7.19% 🔺)
@sentry/browser (incl. Feedback, Feedback Modal)	0 B (removed)
@sentry/browser (incl. Feedback, Feedback Modal, Feedback Screenshot)	0 B (removed)
@sentry/browser (incl. sendFeedback)	26.45 KB (-0.05% 🔽)
@sentry/react	24.35 KB (0%)
@sentry/react (incl. Tracing)	34.31 KB (0%)
@sentry/vue	25.2 KB (0%)
@sentry/vue (incl. Tracing)	33.12 KB (0%)
@sentry/svelte	21.79 KB (0%)
CDN Bundle	23.97 KB (-0.25% 🔽)
CDN Bundle (incl. Tracing)	32.65 KB (-0.18% 🔽)
CDN Bundle (incl. Tracing, Replay)	66.31 KB (-0.09% 🔽)
CDN Bundle (incl. Tracing, Replay, Feedback)	82.09 KB (-0.58% 🔽)
CDN Bundle - uncompressed	70.62 KB (-0.32% 🔽)
CDN Bundle (incl. Tracing) - uncompressed	97.27 KB (-0.24% 🔽)
CDN Bundle (incl. Tracing, Replay) - uncompressed	206.93 KB (-0.11% 🔽)
@sentry/nextjs (client)	33.64 KB (0%)
@sentry/sveltekit (client)	31.9 KB (0%)
@sentry/node	158.85 KB (-0.01% 🔽)
@sentry/browser (incl. FeedbackAsync)	30.93 KB (added)
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed	253.22 KB (added)

billyvg · 2024-04-17T19:33:39Z

It looks like @sentry/browser (incl. Feedback) == @sentry/browser (incl. FeedbackAsync) -- wouldn't be expect the latter to be smaller?

ryan953 · 2024-04-17T20:02:50Z

It looks like @sentry/browser (incl. Feedback) == @sentry/browser (incl. FeedbackAsync) -- wouldn't be expect the latter to be smaller?

ya, i think i need to have different entrypoints, right now it's re-using the same index.ts file.

but i also noticed that feedbackAsyncIntegration() doesn't work in npm land because the window.Sentry member is not set. So i've been looking at that.

ryan953 · 2024-04-18T05:27:43Z

@mydea can you take another look at the async loader helper function? It seems to me that it only works when the SDK is loaded via CDN and window.Sentry exists.
In npm builds we either have to have the code bundled or else it won't work.

Also, I was surprised the sizes are the same even though imports are different for the Feedback and FeedbackAsync variants that the CI posted above and Billy pointed out.

mydea · 2024-04-18T07:50:46Z

@mydea can you take another look at the async loader helper function? It seems to me that it only works when the SDK is loaded via CDN and window.Sentry exists. In npm builds we either have to have the code bundled or else it won't work.

Also, I was surprised the sizes are the same even though imports are different for the Feedback and FeedbackAsync variants that the CI posted above and Billy pointed out.

Hopefully fixed here: #11673

ryan953 · 2024-04-18T16:49:47Z

@billyvg following up, i expected that the size for @sentry/browser (incl. FeedbackAsync) would be 30.94KB and the full incl. Feedback bundle 38.26KB.

Once that's sorted we should be all set imo

ryan953 · 2024-04-19T17:48:25Z

packages/browser/src/feedbackAsync.ts

The bundler seemed like it wasn't tree-shaking properly when this file was combined with src/feedback.ts. Splitting them apart resulted in the bundle-sizes I expected

ryan953 · 2024-04-19T17:49:17Z

packages/browser/src/index.bundle.feedback.ts

-  feedbackScreenshotIntegration,
-  getFeedback,
-} from '@sentry-internal/feedback';
+export { feedbackIntegration } from './feedback';


Note that we're using feedbackIntegration as the default here, not feedbackAsyncIntegration. (same in the othe bundle configs)
So CDN users, by default, won't have async loading.

This is something we can talk about and change outside this PR

ryan953 · 2024-04-19T17:54:29Z

packages/browser/src/feedbackAsync.ts

+
+// This is for users who want to have a lazy-loaded feedback widget
+export const feedbackAsyncIntegration = buildFeedbackIntegration({
+  lazyLoadIntegration,


This whole buildFeedbackIntegration exists, and is called from inside the browser package, so that we can inject lazyLoadIntegration instead of creating a circular dependency between @sentry/browser and @sentry/feedback.

billyvg

LGTM

billyvg · 2024-04-19T18:06:00Z

packages/feedback/src/core/integration.ts

+  // The type here should be `keyof typeof LazyLoadableIntegrations`, but that'll cause a cicrular
+  // dependency with @sentry/core
+  lazyLoadIntegration: (name: 'feedbackModalIntegration' | 'feedbackScreenshotIntegration') => Promise<IntegrationFn>;


not for this PR, but should this type be moved to @sentry/types?

ya, we can do that in a followup. It depends on a list of strings, keyof so might need some copy+paste too.

packages/feedback/src/core/integration.ts

billyvg · 2024-04-19T18:10:10Z

packages/feedback/src/core/integration.ts

+      ]);
+      if (!modalIntegration || (showScreenshot && !screenshotIntegration)) {
+        // trouble!
+        throw new Error('Missing feedback helper integration!');


Do we know how an integration could be missing? Would be helpful to provide a potential solution or a link to one

Idk if we strictly know what was supposed to be setup and failed. Certainly if either is missing then the async loading didn't work.

That could easily happen on purpose if the site relies on async loading but the user is offline right now, for example.

I updated the comment with a TODO. We can for sure add help for the developer so they can make any adjustments, but user-facing i'm not sure where to go with it.

…ding fails

@ryan953

As noticed by @ryan953 here: #11621 (comment), this was not actually working properly in NPM-mode. I added a proper test for this and fixed this, so hopefully should be all good now.

feat(feedback): Create async bundles and code to resolve helper integ…

55500be

…rations

ryan953 requested review from a team and AbhiPrasad April 16, 2024 23:19

ryan953 marked this pull request as ready for review April 17, 2024 21:14

mydea mentioned this pull request Apr 18, 2024

fix(browser): Ensure lazyLoadIntegration works in NPM mode #11673

Merged

ryan953 force-pushed the ryan953/async-bundles branch from f2f0d83 to 55500be Compare April 18, 2024 16:49

poke integration exports to see if tree-shaking is affected

5c5130a

ryan953 force-pushed the ryan953/async-bundles branch from 5c5130a to 55500be Compare April 18, 2024 18:39

ryan953 added 7 commits April 19, 2024 09:58

poke for bundle-size results

75d246a

fix imports and adjust size limits

7ee8631

cleanup some extra lines

238cb5c

make injected integration getters optional

08297f1

keep the size check for sendFeedback

d29f87a

fix size limits

d4e42dc

only need one index file

f59555f

ryan953 commented Apr 19, 2024

View reviewed changes

billyvg approved these changes Apr 19, 2024

View reviewed changes

ryan953 added 5 commits April 19, 2024 11:58

reset showScreenshot default and add todo comments for when async lao…

6e25a73

…ding fails

fix unit test

6200b01

keep the default options obj

c5daa22

cleanup shim exports

b8c60ec

only exporting the "built" integration

0fbf664

ryan953 added 2 commits April 19, 2024 14:03

formatting

4997f25

fix import path for tests

780a936

ryan953 merged commit a6e7324 into develop Apr 21, 2024
96 checks passed

ryan953 deleted the ryan953/async-bundles branch April 21, 2024 23:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(feedback): Create async bundles and code to resolve helper integrations #11621

feat(feedback): Create async bundles and code to resolve helper integrations #11621

ryan953 commented Apr 16, 2024 •

edited

Loading

github-actions bot commented Apr 16, 2024 •

edited

Loading

billyvg commented Apr 17, 2024

ryan953 commented Apr 17, 2024

ryan953 commented Apr 18, 2024

mydea commented Apr 18, 2024

ryan953 commented Apr 18, 2024 •

edited

Loading

ryan953 Apr 19, 2024

ryan953 Apr 19, 2024 •

edited

Loading

ryan953 Apr 19, 2024

billyvg left a comment

billyvg Apr 19, 2024

ryan953 Apr 19, 2024 •

edited

Loading

billyvg Apr 19, 2024

ryan953 Apr 19, 2024

ryan953 Apr 19, 2024

feat(feedback): Create async bundles and code to resolve helper integrations #11621

feat(feedback): Create async bundles and code to resolve helper integrations #11621

Conversation

ryan953 commented Apr 16, 2024 • edited Loading

github-actions bot commented Apr 16, 2024 • edited Loading

size-limit report 📦

billyvg commented Apr 17, 2024

ryan953 commented Apr 17, 2024

ryan953 commented Apr 18, 2024

mydea commented Apr 18, 2024

ryan953 commented Apr 18, 2024 • edited Loading

ryan953 Apr 19, 2024

Choose a reason for hiding this comment

ryan953 Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

ryan953 Apr 19, 2024

Choose a reason for hiding this comment

billyvg left a comment

Choose a reason for hiding this comment

billyvg Apr 19, 2024

Choose a reason for hiding this comment

ryan953 Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

billyvg Apr 19, 2024

Choose a reason for hiding this comment

ryan953 Apr 19, 2024

Choose a reason for hiding this comment

ryan953 Apr 19, 2024

Choose a reason for hiding this comment

ryan953 commented Apr 16, 2024 •

edited

Loading

github-actions bot commented Apr 16, 2024 •

edited

Loading

ryan953 commented Apr 18, 2024 •

edited

Loading

ryan953 Apr 19, 2024 •

edited

Loading

ryan953 Apr 19, 2024 •

edited

Loading