ref(replay): Extract some functions out from class#6448
Conversation
size-limit report 📦
|
6294803 to
54c3a1d
Compare
| // Do not apply replayId to the root event | ||
| if ( | ||
| // @ts-ignore new event type | ||
| event.type === REPLAY_EVENT_NAME |
There was a problem hiding this comment.
m: can we create a issue or add a todo somewhere that adds this event type to the TS types?
Also @JonasBa do we need to do the same for profiling?
There was a problem hiding this comment.
Yes, this is planned as next step - need to write up an issue!
54c3a1d to
78a9df4
Compare
| replay.getContext().traceIds.add(event.contexts.trace.trace_id as string); | ||
| return event; | ||
| } | ||
|
|
||
| // no event type means error | ||
| if (!event.type) { | ||
| replay.getContext().errorIds.add(event.event_id as string); |
There was a problem hiding this comment.
Might be good to add methods to replay to so that we don't expose the data structure/context here.
There was a problem hiding this comment.
Yes, I would like to keep refining the public API surface of this over time. there are a few more places where IMHO we can/should tighten this up! I'll leave this as is for now, but note down to revisit this!
| } | ||
|
|
||
| // Need to collect visited URLs | ||
| replay.getContext().urls.push(result.name); |
There was a problem hiding this comment.
Similar thing to above with trace/error ids.
78a9df4 to
12840da
Compare
| * Create a "span" for the total amount of memory being used by JS objects | ||
| * (including v8 internal objects). | ||
| */ | ||
| export function addMemoryEntry(replay: ReplayContainer): Promise<void[]> | undefined { |
There was a problem hiding this comment.
l: can we change the func signature here to just be void? Then we don't need the return statements and we can also just void createPerformanceSpans
There was a problem hiding this comment.
good finding, there is actually nothing async about this 😅 so should be good to clean up. done!
There was a problem hiding this comment.
addEvent was also sync before, and we are not really waiting for it anywhere?
There was a problem hiding this comment.
🤔 , it may have been a bug -- eventbuffer's addevent is async, looks like replay.addEvent was not returning the promise.
There was a problem hiding this comment.
OK, I think you're right - but I'll make a separate PR to fix that 👍
AbhiPrasad
left a comment
There was a problem hiding this comment.
With some further refactoring in mind, let's 
12840da to
26840af
Compare
26840af to
cf54a67
Compare
This is a first pass of extracting some functions out from the core
replayclass into functions.There is for sure much more refactoring potential there, for now mostly we pass
replayinto the class as arguments, eventually we can def. make this more atomic. But it should be a starting point. There are also more functions to extract, but I figured we can do this in waves.