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

feat(replay): Merge packages together & ensure bundles are built #11552

Merged
merged 10 commits into from
Apr 11, 2024

Conversation

mydea
Copy link
Member

@mydea mydea commented Apr 11, 2024

This reverts #11342 and instead generates multiple bundles etc. from the one @sentry-internal/feedback package.

  • The feedback CDN bundle integration includes the modal integration for now.
  • There is also a dedicated modal & screenshot integration on the CDN
  • Also kept the shims etc. that Ryan added in the now-reverted PR
  • Size limits now seem correct to me! I bumped them (as they now correctly include the feedback modal etc), but you can see the diff to the screenshots being included (it's not too high, really).

@mydea mydea self-assigned this Apr 11, 2024
@mydea mydea changed the title Fn/feedback package feat(replay): Merge packages together & ensure bundles are built Apr 11, 2024
Copy link
Contributor

github-actions bot commented Apr 11, 2024

size-limit report 📦

Path Size
@sentry/browser 22.13 KB (0%)
@sentry/browser (incl. Tracing) 31.85 KB (0%)
@sentry/browser (incl. Tracing, Replay) 67.04 KB (0%)
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 60.63 KB (0%)
@sentry/browser (incl. Tracing, Replay with Canvas) 70.86 KB (0%)
@sentry/browser (incl. Tracing, Replay, Feedback) 80.76 KB (+0.01% 🔺)
@sentry/browser (incl. Feedback) 35.69 KB (+0.03% 🔺)
@sentry/browser (incl. Feedback, Feedback Modal) 35.69 KB (+0.01% 🔺)
@sentry/browser (incl. Feedback, Feedback Modal, Feedback Screenshot) 37.72 KB (+5.67% 🔺)
@sentry/browser (incl. sendFeedback) 26.93 KB (0%)
@sentry/react 24.81 KB (0%)
@sentry/react (incl. Tracing) 34.76 KB (0%)
@sentry/vue 25.68 KB (0%)
@sentry/vue (incl. Tracing) 33.57 KB (0%)
@sentry/svelte 22.26 KB (0%)
CDN Bundle 24.27 KB (+0.07% 🔺)
CDN Bundle (incl. Tracing) 32.9 KB (+0.11% 🔺)
CDN Bundle (incl. Tracing, Replay) 66.4 KB (-0.21% 🔽)
CDN Bundle (incl. Tracing, Replay, Feedback) 82.57 KB (+2.47% 🔺)
CDN Bundle - uncompressed 72.23 KB (+0.12% 🔺)
CDN Bundle (incl. Tracing) - uncompressed 98.55 KB (+0.09% 🔺)
CDN Bundle (incl. Tracing, Replay) - uncompressed 207.94 KB (+0.04% 🔺)
@sentry/nextjs (client) 34.01 KB (0%)
@sentry/sveltekit (client) 32.37 KB (0%)
@sentry/node 120.19 KB (0%)

Copy link
Member

@lforst lforst left a comment

Choose a reason for hiding this comment

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

Nice thank you!

@mydea mydea force-pushed the fn/feedback-package branch 3 times, most recently from 714312e to b49969f Compare April 11, 2024 11:11
@billyvg
Copy link
Member

billyvg commented Apr 11, 2024

can we add entries in size-limit for only modal and only screenshot? just want to make sure their individual builds are ok too

@@ -5,7 +5,7 @@ window.Sentry = Sentry;
Sentry.init({
dsn: 'https://public@dsn.ingest.sentry.io/1337',
replaysOnErrorSampleRate: 1.0,
replaysSessionSampleRate: 0,
replaysSessionSampleRate: 1.0,
Copy link
Member

Choose a reason for hiding this comment

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

I think this was intentional to make sure that replay is captured when in buffering mode

Copy link
Member Author

Choose a reason for hiding this comment

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

it is not actually doing this today, I believe? At least the test failed 🤔 I merged this in now so we are unblocked, we can revert/tweak this a bit in a follow up!

@mydea mydea force-pushed the fn/feedback-package branch from b49969f to 4bc8457 Compare April 11, 2024 14:09
...makeBundleConfigVariants(
makeBaseBundleConfig({
bundleType: 'addon',
entrypoints: ['src/modal/integration.tsx'],
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
entrypoints: ['src/modal/integration.tsx'],
entrypoints: ['src/modal/integration.ts'],

Copy link
Member Author

Choose a reason for hiding this comment

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

it is actually tsx 😅

Copy link
Member

Choose a reason for hiding this comment

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

oh I didn't know that! that's interesting

@mydea mydea merged commit 621b691 into develop Apr 11, 2024
96 checks passed
@mydea mydea deleted the fn/feedback-package branch April 11, 2024 14:41
cadesalaberry pushed a commit to cadesalaberry/sentry-javascript that referenced this pull request Apr 19, 2024
…sentry#11552)

This reverts getsentry#11342
and instead generates multiple bundles etc. from the one
`@sentry-internal/feedback` package.

* The `feedback` CDN bundle integration includes the modal integration
for now.
* There is also a dedicated modal & screenshot integration on the CDN
* Also kept the shims etc. that Ryan added in the now-reverted PR
* Size limits now seem correct to me! I bumped them (as they now
correctly include the feedback modal etc), but you can see the diff to
the screenshots being included (it's not too high, really).
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.

4 participants