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.

Sign up for GitHub

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

Jump to bottom

ref(replay): Move @sentry/replay code to @sentry-internal/replay #11200

Merged
merged 4 commits into from
Mar 22, 2024
Merged

ref(replay): Move @sentry/replay code to @sentry-internal/replay #11200

merged 4 commits into from
Mar 22, 2024

Conversation

s1gr1d
Copy link
Member

@s1gr1d s1gr1d commented Mar 19, 2024

closes #9165

@s1gr1d s1gr1d force-pushed the sig-move-replay-to-internal branch from 0a56cb3 to ccee1e2 Compare March 19, 2024 14:34
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Mar 19, 2024 •
edited

size-limit report 📦

Path Size
@sentry/browser (incl. Tracing, Replay, Feedback) 80.9 KB (added)
@sentry/browser (incl. Tracing, Replay) 72.24 KB (added)
@sentry/browser (incl. Tracing, Replay with Canvas) 76.04 KB (added)
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 65.81 KB (added)
@sentry/browser (incl. Tracing) 36.83 KB (added)
@sentry/browser (incl. browserTracingIntegration) 36.83 KB (added)
@sentry/browser (incl. feedbackIntegration) 31.35 KB (added)
@sentry/browser (incl. feedbackModalIntegration) 31.47 KB (added)
@sentry/browser (incl. feedbackScreenshotIntegration) 31.48 KB (added)
@sentry/browser (incl. sendFeedback) 27.43 KB (added)
@sentry/browser 22.6 KB (added)
CDN Bundle (incl. Tracing, Replay, Feedback) 75.21 KB (added)
CDN Bundle (incl. Tracing, Replay) 70.05 KB (added)
CDN Bundle (incl. Tracing) 36.41 KB (added)
CDN Bundle 23.97 KB (added)
CDN Bundle (incl. Tracing, Replay) - uncompressed 220.04 KB (added)
CDN Bundle (incl. Tracing) - uncompressed 109.98 KB (added)
CDN Bundle - uncompressed 71 KB (added)
@sentry/react (incl. Tracing, Replay) 72.22 KB (added)
@sentry/react 22.63 KB (added)

s1gr1d
s1gr1d commented Mar 19, 2024
View reviewed changes
MIGRATION.md
Comment on lines +905 to +917
- [Change of Replay default options (`unblock` and `unmask`)](./MIGRATION.md#change-of-replay-default-options-unblock-and-unmask)
- [Angular Tracing Decorator renaming](./MIGRATION.md#angular-tracing-decorator-renaming)
Copy link
Member Author

Choose a reason for hiding this comment

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

I saw that there is a TOC for every sub-heading so I added the missing ones :)

@s1gr1d s1gr1d force-pushed the sig-move-replay-to-internal branch from 76262a0 to d86ab94 Compare March 19, 2024 15:23
@s1gr1d s1gr1d marked this pull request as ready for review March 19, 2024 15:24
@s1gr1d s1gr1d requested review from Lms24, AbhiPrasad and lforst and removed request for Lms24 March 19, 2024 15:24
Lms24
Lms24 reviewed Mar 21, 2024
View reviewed changes
Copy link
Member

@Lms24 Lms24 left a comment •
edited

Choose a reason for hiding this comment

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

So I think we need to split up and target parts of this PR to the v7 branch. Deprecating exports from @sentry/replay on develop won't really do anything because we're going to remove this package anyway in v8.

So I suggest we

  1. introduce the deprecations in v7. I think just the deprecation in v7 should be enough because the replacement for users is to directly import the APIs and types from @sentry/browser (or SDKs building on top of browser). The internal package should just be for us.
  2. Rename the @sentry/replay package to @sentry-internal/replay package in develop (v8).

As an alternative, if you prefer, we can also already introduce this package in v7 but we'd need to worry about releasing it in v7 correctly then, too.

WDYT, does this make sense?

packages/replay-internal/.eslintignore Outdated
Comment on lines 4 to 5
# TODO: Check if we can re-introduce linting in demo
demo
Copy link
Member

Choose a reason for hiding this comment

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

ahh right there's a demo 😅 I think it's fine to leave it as-is

packages/replay/src/index.ts Outdated
Comment on lines 20 to 21
/** @deprecated Import from `@sentry-internal/replay` */
export const getReplay = internalGetReplay;
Copy link
Member

Choose a reason for hiding this comment

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

m: Since this is (potentially) user-facing, the replacement should be to import from @sentry/browser or framework SDK instead of from @sentry-internal/replay.

For us internally, it's fine to eslint-ignore the deprecations in v7 as long as we don't have the @sentry-internal/replay package.

This was referenced Mar 22, 2024
Merged
Closed
@s1gr1d s1gr1d force-pushed the sig-move-replay-to-internal branch from d86ab94 to 5772fbd Compare March 22, 2024 09:43
s1gr1d
s1gr1d commented Mar 22, 2024
View reviewed changes
dev-packages/e2e-tests/verdaccio-config/config.yaml
Comment on lines -125 to -129
'@sentry/replay':
access: $all
publish: $all
unpublish: $all
# proxy: npmjs # Don't proxy for E2E tests!
Copy link
Member Author

Choose a reason for hiding this comment

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

I was able to just delete this (instead of renaming), as there is '@sentry-internal/*' further down.

@s1gr1d s1gr1d requested a review from Lms24 March 22, 2024 10:14
Lms24
Lms24 approved these changes Mar 22, 2024
View reviewed changes
Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

LGTM!

@s1gr1d s1gr1d merged commit 1c10b30 into develop Mar 22, 2024
93 checks passed
@s1gr1d s1gr1d deleted the sig-move-replay-to-internal branch March 22, 2024 13:23
@s1gr1d s1gr1d mentioned this pull request Mar 22, 2024
Closed
s1gr1d added a commit that referenced this pull request Mar 22, 2024
@s1gr1d
ref(v7/replay): Deprecate @sentry/replay exports (#11242)
ef37773 
ref #11200
on v8 #11243
@billyvg
Copy link
Member

billyvg commented Mar 22, 2024

I'm having issues testing this new replay package locally (e.g. in sentry). When I yarn, it complains because the package isn't published to npm:

error Couldn't find package "@sentry-internal/replay@8.0.0-alpha.5" required by "@sentry/browser@file:.yalc/@sentry/browser" on the "npm" registry.

c298lee pushed a commit that referenced this pull request Mar 22, 2024
@s1gr1d @c298lee
ref(replay): Move @sentry/replay code to @sentry-internal/replay (#…
28d5ebe 
…11200)

closes #9165
c298lee pushed a commit that referenced this pull request Mar 23, 2024
@s1gr1d @c298lee
ref(replay): Move @sentry/replay code to @sentry-internal/replay (#…
20cf5e1 
…11200)

closes #9165
cadesalaberry pushed a commit to cadesalaberry/sentry-javascript that referenced this pull request Apr 19, 2024
@s1gr1d @cadesalaberry
ref(replay): Move @sentry/replay code to @sentry-internal/replay (g…
2e4e862 
…etsentry#11200)

closes getsentry#9165
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Lms24 Lms24 Lms24 approved these changes

@AbhiPrasad AbhiPrasad Awaiting requested review from AbhiPrasad

@lforst lforst Awaiting requested review from lforst

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Move @sentry/replay code to @sentry-internal/replay
3 participants
@s1gr1d @billyvg @Lms24