-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
chore: Add missing fields into Replay NetworkRequestData type #8284
Conversation
I think this is not what you think it is :D You are probably looking for |
@mydea What i'm trying to do is import types into sentry so i can have well-typed breadcrumbs/attachments/segments/frames (whatever they're called now) within the ui. this the the data that comes back from the /recording-segments/ api endpoint. |
@ryan953 I can fix this up, what branch on sentry can I test on? |
@billyvg I copy+pasted all the types into |
@billyvg I'm also waiting to merge those two linked PRs after the next SDK bump, so I don't need to copy+paste all the types in. So no rush. |
5b7d63d
to
732589e
Compare
732589e
to
13092c3
Compare
size-limit report 📦
|
packages/replay/src/types/index.ts
Outdated
@@ -2,3 +2,4 @@ export * from './performance'; | |||
export * from './replay'; | |||
export * from './replayFrame'; | |||
export * from './rrweb'; | |||
export type { EventType } from '@sentry-internal/rrweb'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took that duplicate definition out, as you saw below.
Now getsentry/sentry-javascript is importing from '@sentry-internal/rrweb'
and getsentry/sentry will import from getsentry/sentry-javascript
hopefully i can skip @sentry-internal/rrweb altogether if i've cross referenced everything correctly.
|
||
interface NetworkRequestFrame extends BaseSpanFrame { | ||
data: NetworkRequestData | ReplayNetworkRequestData; | ||
op: 'resource.fetch' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about xhr?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think xhr payloads are shaped like ResourceData
instead.
…ReplayNetworkRequestOrResponse added into NetworkRequestData
packages/replay/src/types/index.ts
Outdated
@@ -2,3 +2,4 @@ export * from './performance'; | |||
export * from './replay'; | |||
export * from './replayFrame'; | |||
export * from './rrweb'; | |||
export type { EventType } from '@sentry-internal/rrweb'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took that duplicate definition out, as you saw below.
Now getsentry/sentry-javascript is importing from '@sentry-internal/rrweb'
and getsentry/sentry will import from getsentry/sentry-javascript
hopefully i can skip @sentry-internal/rrweb altogether if i've cross referenced everything correctly.
networkRequestHasHeaders: boolean; | ||
networkResponseHasHeaders: boolean; | ||
sessionSampleRate: number; | ||
useCompression: boolean; | ||
useCompressionOption: boolean; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorted these fields, no changes.
this object is duplicated in sentry, so i'm happy for the re-use here.
|
||
interface NetworkRequestFrame extends BaseSpanFrame { | ||
data: NetworkRequestData | ReplayNetworkRequestData; | ||
op: 'resource.fetch' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think xhr payloads are shaped like ResourceData
instead.
308fc44
to
69f73ee
Compare
0d0fefb
to
69f73ee
Compare
@@ -1,4 +1,5 @@ | |||
export * from './performance'; | |||
export * from './replay'; | |||
export * from './replayFrame'; | |||
export * from './request'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new file to prevent circular refs.
Huh, tests did not actually pass here - it seems it cancelled on |
Before submitting a pull request, please take a look at our
Contributing guidelines and verify:
yarn lint
) & (yarn test
).