fix: Use correct types for event context data #2910
Conversation
kamilogorek
force-pushed
the
context-type
branch
from
45f14a0 to
933f399
Compare
kamilogorek
force-pushed
the
context-type
branch
from
933f399 to
150f7f2
Compare
size-limit report
|
kamilogorek
force-pushed
the
context-type
branch
from
d17146a to
86042df
Compare
kamilogorek
force-pushed
the
context-type
branch
from
86042df to
9fc9d6d
Compare
kamilogorek
force-pushed
the
context-type
branch
from
82cd860 to
56e2c62
Compare
| @@ -853,7 +853,7 @@ export class Tracing implements Integration { | |||
| Tracing._log(`[Tracing] Transaction: ${SpanStatus.Cancelled} since it maxed out maxTransactionDuration`); | |||
| if (event.contexts && event.contexts.trace) { | |||
| event.contexts.trace = { | |||
| ...event.contexts.trace, | |||
| ...(typeof event.contexts.trace === 'object' && event.contexts.trace), | |||
There was a problem hiding this comment.
Why do we now do a type check here?
There was a problem hiding this comment.
Because unknown is more restrictive than any, and requires type check before it can be spread. On the other hand, it doesnt require unnecessary no-explcit-any linting rule.
There was a problem hiding this comment.
event.contexts.trace has type Context, it must be an object, mustn't it?
Co-authored-by: Rodolfo Carvalho <[email protected]>
| public setContext(key: string, context: Context | null): this { | ||
| if (context === null) { | ||
| // eslint-disable-next-line @typescript-eslint/no-dynamic-delete | ||
| delete this._contexts[key]; |
There was a problem hiding this comment.
Hmm this is more than changing the types, would the previous code delete keys?! Was this a bug?
There was a problem hiding this comment.
From https://develop.sentry.dev/sdk/unified-api/
So now setting a context to null deletes it. Let's document this in the changelog.
There was a problem hiding this comment.
Was a bug, fixed.
per #2905 (comment)