ref(feedback): Create stub integrations for feedback modal & screenshot integrations #11342
Conversation
ryan953
force-pushed
the
ryan953/feedback-integration-pkg-stubs
branch
from
6d8355e to
7ce768f
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| walk(filePath); | ||
| } else { | ||
| if (filePath.endsWith('.d.ts')) { | ||
| const content = fs.readFileSync(filePath, 'utf8'); |
Check failure
Code scanning / CodeQL
Potential file system race condition
| const newContent = content.replace(preactImportRegex, '// replaced import from preact'); | ||
|
|
||
| // and write the new content to the file | ||
| fs.writeFileSync(filePath, snippet + newContent, 'utf8'); |
Check failure
Code scanning / CodeQL
Potential file system race condition
| walk(filePath); | ||
| } else { | ||
| if (filePath.endsWith('.d.ts')) { | ||
| const content = fs.readFileSync(filePath, 'utf8'); |
Check failure
Code scanning / CodeQL
Potential file system race condition
| const newContent = content.replace(preactImportRegex, '// replaced import from preact'); | ||
|
|
||
| // and write the new content to the file | ||
| fs.writeFileSync(filePath, snippet + newContent, 'utf8'); |
Check failure
Code scanning / CodeQL
Potential file system race condition
Bundle ReportChanges will decrease total bundle size by 23.53kB ⬇️
|
size-limit report 📦
|
ryan953
force-pushed
the
ryan953/feedback-integration-pkg-stubs
branch
2 times, most recently
from
ed3c5d5 to
47f178a
Compare
…ot code ot be broken out of the main integration
ryan953
force-pushed
the
ryan953/feedback-integration-pkg-stubs
branch
from
47f178a to
b360ead
Compare
|
Just to clarify: Do we really want people to add a separate integration for the feedback modal? Should that not be the default experience? |
@mydea I was thinking that aiming for the separate bundle is a good idea because it gives us options related to bundle size. Reverting the choice is super easy to do, but I haven't got the best metrics on how many bytes this 'many integration' approach will be. I'd like to compare the v8 version of Bundle wise I think there are two groups of sentry users:
I think bundle size is important with feedback in general because most people visiting a site won't click the "Give Feedback" button, so in most cases the modal will be extra bytes. Especially on mobile for example, where people might feel it more. |
|
What if we had:
|
|
@billyvg I think any of these combinations are possible with the lego bits we have. Especially if we have some combinations available through the Loader, and some not. I think the info we really need is bundle size measurements so we can better make a type-1 hard-to-undo decision related to the function signatures. The purpose after all is to lazy load some of the things, without that requirement then we wouldn't need to mess with the function signatures at all. |
|
Is there a reason to have the feedback integrations (however many we end up with) in separate packages other than being able to generate multiple bundles? This should be possible within one package by specifying multiple rollup config objects with separate entry points. I don't have a strong opinion about what is better but I'd like to avoid a situation in which we'd get circular dependencies between the feedback packages if they import from each other. |
@Lms24 I think it'll be better to have things split up. All the code is together right now in |
|
@Lms24 @mydea @billyvg I wrote some ideas down and put them into #11435. I'm trying to sketch out the overall vision and big steps that'll get us there. The most important next step is keeping a v8-beta release unblocked, so I'll put up the PR for that, but afterwards this PR is lays down the foundation to build on top of. Thanks to billy also for the api help & thinking through the bundle & imports business. |
…the exported surface area (#11355) You can read this commit-by-commit to watch the refactor unfold The situation is: after #11342 is merged, all the code inside each of the `packages/feedback/src/[core|modal|screenshot]` will be split up into separate integrations. These integrations can't share the same `src/types/*.ts` definitions anymore. Therefore type definitions will need to live in @sentry/types instead. This PR moves all the types, and since they'll be public now in that package, i refactored things to reduce the public surface area and improve names where possible. The only non-type changes in here are where we move `createDialog.ts` and `createInput.ts` into their respective `integration.ts` files, no code changes at all. Related to #11435
ryan953
force-pushed
the
ryan953/feedback-integration-pkg-stubs
branch
from
f1f5f17 to
d5e82e5
Compare
ryan953
force-pushed
the
ryan953/feedback-integration-pkg-stubs
branch
from
d5e82e5 to
0a98f8f
Compare
) 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).
…the exported surface area (getsentry#11355) You can read this commit-by-commit to watch the refactor unfold The situation is: after getsentry#11342 is merged, all the code inside each of the `packages/feedback/src/[core|modal|screenshot]` will be split up into separate integrations. These integrations can't share the same `src/types/*.ts` definitions anymore. Therefore type definitions will need to live in @sentry/types instead. This PR moves all the types, and since they'll be public now in that package, i refactored things to reduce the public surface area and improve names where possible. The only non-type changes in here are where we move `createDialog.ts` and `createInput.ts` into their respective `integration.ts` files, no code changes at all. Related to getsentry#11435
…ot integrations (getsentry#11342) We're planning to split up the modal and screenshot functionality of the feedback widget into their own integrations, so they can be loaded async if folks want to do that. Loading async would have the benefit that the code is only needed when the user interacts with the feedback widget, and we're actually going to render the modal whether that be with or without the screenshot input/editor. This PR sets up some stubs for where the code will live. There's a bunch of boilerplate for the new integrations inside packages/* and also lots of exports to setup so that the stubs are available to be imported into apps. Currently if someone wanted to setup feedback with screenshots they would be doing: ```javascript import * as Sentry from "@sentry/browser"; import {feedbackIntegration, feedbackModalIntegration, feedbackScreenshotIntegration} from '@sentry-internal/feedback'; ... integrations: [ feedbackIntegration(), feedbackModalIntegration(), feedbackScreenshotIntegration(), ] ``` **After this PR people will keep doing that.** In a followup PR we'll move the implementation from `@sentry-internal/feedback` into the new `@sentry-internal/feedback-modal` and `@sentry-internal/feedback-screenshot`, so people will be able to do this instead: ```diff import * as Sentry from "@sentry/browser"; - import {feedbackIntegration, feedbackModalIntegration, feedbackScreenshotIntegration} from '@sentry-internal/feedback'; + import {feedbackIntegration} from '@sentry-internal/feedback'; + import {feedbackModalIntegration} from '@sentry-internal/feedback-modal'; + import {feedbackScreenshotIntegration} from '@sentry-internal/feedback-screenshot'; ... integrations: [ feedbackIntegration(), feedbackModalIntegration(), feedbackScreenshotIntegration(), ] ``` Equally valid, is people will be able to import everything from `@sentry/browser` too, ie `import * as Sentry from "@sentry/browser";` with `Sentry.feedbackScreenshotIntegration()` Related to getsentry#11435
…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).
We're planning to split up the modal and screenshot functionality of the feedback widget into their own integrations, so they can be loaded async if folks want to do that. Loading async would have the benefit that the code is only needed when the user interacts with the feedback widget, and we're actually going to render the modal whether that be with or without the screenshot input/editor.
This PR sets up some stubs for where the code will live. There's a bunch of boilerplate for the new integrations inside packages/* and also lots of exports to setup so that the stubs are available to be imported into apps.
Currently if someone wanted to setup feedback with screenshots they would be doing:
After this PR people will keep doing that.
In a followup PR we'll move the implementation from
@sentry-internal/feedbackinto the new@sentry-internal/feedback-modaland@sentry-internal/feedback-screenshot, so people will be able to do this instead:Equally valid, is people will be able to import everything from
@sentry/browsertoo, ieimport * as Sentry from "@sentry/browser";withSentry.feedbackScreenshotIntegration()Related to #11435