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: Export getCanvasManager & allow passing it to record() #122

Merged
merged 4 commits into from
Oct 30, 2023

Conversation

mydea
Copy link
Member

@mydea mydea commented Oct 25, 2023

This PR exports a new getCanvasManager() method which can be used to dynamically pass a canvas manager, allowing tree shaking.

This also removes the __RRWEB_EXCLUDE_CANVAS__ build flag - canvas will always be excluded now by default.

Expected usage:

import { record, getCanvasManager } from '@sentry-internal/canvas';

record({
  // other config...
  getCanvasManager,
});

The idea is that we can expose this somehow (?) from replay, so users can do e.g.:

import { Replay, getReplayCanvasManager } from '@sentry/browser';

Sentry.init({
  integrations: [
    new Replay({ canvasManager: getReplayCanvasManager() })
  ]
});

Or something like this, allowing people to opt-in to canvas recording at runtime, vs requiring a specific build step for it.

@mydea mydea requested a review from billyvg October 25, 2023 11:10
@mydea mydea self-assigned this Oct 25, 2023
Copy link
Member

@billyvg billyvg left a comment

Choose a reason for hiding this comment

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

Can we make this default? (e.g. get rid of build flag?)

@mydea mydea force-pushed the fn/custom-exports branch 2 times, most recently from 39ceb88 to c22a3e8 Compare October 27, 2023 08:01
Base automatically changed from fn/custom-exports to sentry-v2 October 27, 2023 08:10
@mydea mydea force-pushed the fn/export-canvas-manager branch from e7e0bc7 to 8352f64 Compare October 27, 2023 08:14
@mydea mydea marked this pull request as ready for review October 27, 2023 08:14
@mydea
Copy link
Member Author

mydea commented Oct 27, 2023

Can we make this default? (e.g. get rid of build flag?)

Yeah, I guess that makes sense here! 👍

@github-actions
Copy link

github-actions bot commented Oct 27, 2023

size-limit report 📦

Path Size
rrweb - record only (gzipped) 16.6 KB (-26.27% 🔽)
rrweb - record only (min) 57.17 KB (-27.45% 🔽)
rrweb - record with treeshaking flags (gzipped) 15.35 KB (+0.22% 🔺)
rrweb - record & getCanvasManager only (gzipped) 22.54 KB (added)

@mydea mydea force-pushed the fn/export-canvas-manager branch from 3c1d07d to a77e2b9 Compare October 27, 2023 08:53
@mydea mydea force-pushed the fn/export-canvas-manager branch from 0c4a6cd to f757a63 Compare October 27, 2023 11:35
@mydea mydea force-pushed the fn/export-canvas-manager branch from 968a2fc to 5c9398f Compare October 27, 2023 11:40
@mydea
Copy link
Member Author

mydea commented Oct 27, 2023

OK, I fixed the tests, seems good now. I ended up changing it so that you have to pass in a getCanvasManager method instead of the instance, which is a slightly "worse" API, but easier to implement because we need access to some of the parsed options from record, which we don't easily have otherwise 😬

@mydea mydea requested a review from billyvg October 30, 2023 08:45
@mydea mydea merged commit 850a14b into sentry-v2 Oct 30, 2023
@mydea mydea deleted the fn/export-canvas-manager branch October 30, 2023 14:57
mydea added a commit to getsentry/sentry-javascript that referenced this pull request Oct 31, 2023
- feat: Export getCanvasManager & allow passing it to record() [#122](getsentry/rrweb#122)
- feat: Remove hooks related code, which is not used [#126](getsentry/rrweb#126)
- feat: Remove plugins related code, which is not used [#123](getsentry/rrweb#123)
- feat: Refactor module scope vars & export mirror & `takeFullSnapshot` directly [#113](getsentry/rrweb#113)
- fix(rrweb): Fix rule.style being undefined [#121](getsentry/rrweb#121)
- ref: Avoid unnecessary cloning of objects or arrays [#125](getsentry/rrweb#125)
- ref: Avoid cloning events to add timestamp [#124](getsentry/rrweb#124)
mydea added a commit to getsentry/sentry-javascript that referenced this pull request Oct 31, 2023
- feat: Export getCanvasManager & allow passing it to record() [#122](getsentry/rrweb#122)
- feat: Remove hooks related code, which is not used [#126](getsentry/rrweb#126)
- feat: Remove plugins related code, which is not used [#123](getsentry/rrweb#123)
- feat: Refactor module scope vars & export mirror & `takeFullSnapshot` directly [#113](getsentry/rrweb#113)
- fix(rrweb): Fix rule.style being undefined [#121](getsentry/rrweb#121)
- ref: Avoid unnecessary cloning of objects or arrays [#125](getsentry/rrweb#125)
- ref: Avoid cloning events to add timestamp [#124](getsentry/rrweb#124)
mydea added a commit to getsentry/sentry-javascript that referenced this pull request Oct 31, 2023
- feat: Export getCanvasManager & allow passing it to record() [#122](getsentry/rrweb#122)
- feat: Remove hooks related code, which is not used [#126](getsentry/rrweb#126)
- feat: Remove plugins related code, which is not used [#123](getsentry/rrweb#123)
- feat: Refactor module scope vars & export mirror & `takeFullSnapshot` directly [#113](getsentry/rrweb#113)
- fix(rrweb): Fix rule.style being undefined [#121](getsentry/rrweb#121)
- ref: Avoid unnecessary cloning of objects or arrays [#125](getsentry/rrweb#125)
- ref: Avoid cloning events to add timestamp [#124](getsentry/rrweb#124)
mydea added a commit to getsentry/sentry-javascript that referenced this pull request Oct 31, 2023
- feat: Export getCanvasManager & allow passing it to record()
[#122](getsentry/rrweb#122)
- feat: Remove hooks related code, which is not used
[#126](getsentry/rrweb#126)
- feat: Remove plugins related code, which is not used
[#123](getsentry/rrweb#123)
- feat: Refactor module scope vars & export mirror & `takeFullSnapshot`
directly [#113](getsentry/rrweb#113)
- fix(rrweb): Fix rule.style being undefined
[#121](getsentry/rrweb#121)
- ref: Avoid unnecessary cloning of objects or arrays
[#125](getsentry/rrweb#125)
- ref: Avoid cloning events to add timestamp
[#124](getsentry/rrweb#124)


Note: With this update, canvas is _always_ excluded, unless we opt in by
passing a `getCanvasManager` function to `record()`. We'll provide a way
to do this once we have a fully formed canvas story. For now, this will
reduce bundle size considerably for all SDK users.
billyvg pushed a commit that referenced this pull request Apr 26, 2024
This PR exports a new `getCanvasManager()` method which can be used to
dynamically pass a canvas manager, allowing tree shaking.

This also removes the `__RRWEB_EXCLUDE_CANVAS__` build flag - canvas
will _always_ be excluded now by default.

Expected usage:

```js
import { record, getCanvasManager } from '@sentry-internal/canvas';

record({
  // other config...
  getCanvasManager,
});
```

The idea is that we can expose this somehow (?) from replay, so users
can do e.g.:

```js
import { Replay, getReplayCanvasManager } from '@sentry/browser';

Sentry.init({
  integrations: [
    new Replay({ canvasManager: getReplayCanvasManager() })
  ]
});
```

Or something like this, allowing people to opt-in to canvas recording at
runtime, vs requiring a specific build step for it.
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.

2 participants