Skip to content

feat(replay): Change addEvent to be async #6695

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
Merged
merged 9 commits into from
Jan 11, 2023
Merged

feat(replay): Change addEvent to be async #6695

merged 9 commits into from
Jan 11, 2023

Conversation

billyvg
Copy link
Member

@billyvg billyvg commented Jan 9, 2023 •
edited
Loading

addEvent should be async as it waits for a response from Compression worker. We don't necessarily always await addEvent(), but it's more correct that it is marked as async and can maybe save some headaches down the line.

Requires #6696

@github-actions
Copy link
Contributor

github-actions bot commented Jan 9, 2023 •
edited
Loading

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 19.84 KB (-0.01% 🔽)
@sentry/browser - ES5 CDN Bundle (minified) 61.46 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.62 KB (+0.02% 🔺)
@sentry/browser - ES6 CDN Bundle (minified) 55 KB (0%)
@sentry/browser - Webpack (gzipped + minified) 20.38 KB (0%)
@sentry/browser - Webpack (minified) 66.55 KB (0%)
@sentry/react - Webpack (gzipped + minified) 20.4 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified) 47.63 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 26.82 KB (+0.02% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 25.25 KB (-0.01% 🔽)
@sentry/replay ES6 CDN Bundle (gzipped + minified) 43.09 KB (+0.11% 🔺)
@sentry/replay - Webpack (gzipped + minified) 38.29 KB (+0.09% 🔺)

@billyvg billyvg force-pushed the feat-replay-change-addevent-to-be-async branch from db689d6 to da00da3 Compare January 9, 2023 20:15
@billyvg billyvg changed the base branch from master to ref-replay-allow-timestamp-as-argument-send-request January 9, 2023 20:15
billyvg
billyvg commented Jan 9, 2023
View reviewed changes
packages/replay/src/types.ts Outdated
}

export type WorkerAddEventResponse = boolean;
Copy link
Member Author

Choose a reason for hiding this comment

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

Ehh wouldn't mind a suggestion for a better name for this type alias. Right now it's just boolean for true/false if worker succeeds in processing the event, but could change in the future (e.g. we could return the current compressed size).

Copy link
Member

Choose a reason for hiding this comment

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

IMHO this should return an object, otherwise it is hard to extend this in the future.
But actually, I wonder if we really need this at all. If all we care about is if it has been successfully added or not, it would be more appropriate to just let it reject the promise than to resolve with true.

@billyvg billyvg force-pushed the feat-replay-change-addevent-to-be-async branch from da00da3 to 641c08c Compare January 10, 2023 00:48
This was referenced Jan 10, 2023
Merged
Closed
@billyvg billyvg marked this pull request as ready for review January 10, 2023 04:05
@billyvg
Copy link
Member Author

billyvg commented Jan 10, 2023

Huh what's wrong with CI here:

image

image

@billyvg billyvg requested review from Lms24 and mydea January 10, 2023 04:08
mydea
mydea reviewed Jan 10, 2023
View reviewed changes
Copy link
Member

@mydea mydea left a comment

Choose a reason for hiding this comment

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

IMHO instead of having a promise that resolves in true/false, I would just let the promise reject if we couldn't add the event. Then we can change the API to addEvent: Promise<void>, which is IMHO nicer than Promise<boolean>.

packages/replay/src/types.ts Outdated
}

export type WorkerAddEventResponse = boolean;
Copy link
Member

Choose a reason for hiding this comment

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

IMHO this should return an object, otherwise it is hard to extend this in the future.
But actually, I wonder if we really need this at all. If all we care about is if it has been successfully added or not, it would be more appropriate to just let it reject the promise than to resolve with true.

@billyvg billyvg requested a review from mydea January 10, 2023 14:04
mydea
mydea approved these changes Jan 10, 2023
View reviewed changes
Copy link
Member

@mydea mydea left a comment

Choose a reason for hiding this comment

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

LGTM!

@billyvg billyvg force-pushed the ref-replay-allow-timestamp-as-argument-send-request branch from dec280a to d0a164e Compare January 11, 2023 14:42
@billyvg billyvg force-pushed the feat-replay-change-addevent-to-be-async branch from 7bee5e4 to c400bb0 Compare January 11, 2023 15:16
Base automatically changed from ref-replay-allow-timestamp-as-argument-send-request to master January 11, 2023 15:19
@billyvg billyvg force-pushed the feat-replay-change-addevent-to-be-async branch from c400bb0 to 024c4ca Compare January 11, 2023 15:20
@billyvg billyvg force-pushed the feat-replay-change-addevent-to-be-async branch from 2bed985 to 97cb8ea Compare January 11, 2023 15:51
@billyvg billyvg merged commit 770a33d into master Jan 11, 2023
@billyvg billyvg deleted the feat-replay-change-addevent-to-be-async branch January 11, 2023 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mydea mydea mydea approved these changes

@Lms24 Lms24 Awaiting requested review from Lms24

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

Successfully merging this pull request may close these issues.
2 participants
@billyvg @mydea