fix(feedback): Inject preact from feedbackModal into feedbackScreenshot integration #12535
Conversation
It'd be good to have the bundle-size check again. i expect if we're fixing the problem correctly, size will be smaller for the screenshot portion |
size-limit report 📦
|
|
Welp, bundle sizes are about the same, up by ~420bytes. I guess that's the overhead of the extra variable passing? If we look at the bundled files (esm, easier to see) we can see that feedback-modal has preact inside it, and feedback-screenshot does not: See line 1206 in feedback-modal.tsx which includes This is what I expected, but |
|
aha! We can't trust these bundle numbers because above for what we're doing here. Any bundle in the size-limit report above with "Feedback" in it will include all code. It's expected that stuff will be tree-shaken if modal & screenshots are not needed or async loaded. I added a case to https://github.com/getsentry/sentry-javascript/blob/70e942d6426da9f8ce1ed1a267e266620e07da24/packages/feedback/rollup.bundle.config.mjs to compare the bundle sizes of all three integrations directly, ran it before and after this change. this is the new snippet
...makeBundleConfigVariants(
makeBaseBundleConfig({
bundleType: 'addon',
entrypoints: ['src/core/integration.ts'],
jsVersion: 'es6',
licenseTitle: '@sentry-internal/feedback-core',
outputFileBase: () => 'bundles/feedback-core',
sucrase: {
// The feedback widget is using preact so we need different pragmas and jsx runtimes
jsxPragma: 'h',
jsxFragmentPragma: 'Fragment',
jsxRuntime: 'classic',
},
}),
),
The numbers for the built files are:
So by those numbers we should be shifting some bytes from screenshots into modal, but saving 2kb overall. Assuming tree-shaking is happening. If there's no tree-shaking then folks shouldn't use the async bundle at all, they'll get the full bundle, which is represented in the size-limit report above. |
…12784) This broke the `Add a screenshot` button on the user feedback, see #12713 This has been explicitly removed in favor of injecting `h` in #12535 but at that point it seems to be too late to do. Co-authored-by: Ryan Albrecht <[email protected]>
I noticed this after #12784, but the `<CropCorner>` wasn't inside the `ScreenshotEditorFactory()` function. So of course it wasn't getting access to the `h` ref that is passed in. That variable is what the `<div>` gets transpiled into -> it becomes `h.createElement('div')`. So what i'm doing is moving `CropCorner` into a `CropCornerFactory` so we can pass `h` in and hopefully not have the extra `import .. from 'preact';` in the 2nd bundle. Related to #12535
I believe the error we're seeing in #12534 is from having preact bundled twice: once into the Modal integration, and then again into the Screenshot one.
This PR updates the Screenshot code so that it requires preact to be injected into it, which should reduce the bundle size of that integration, but also solve the bug because there will only be one instance of preact and it's state within the sdk.
Fixes #12534