-
Notifications
You must be signed in to change notification settings - Fork 195
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
fix: startup flush policy should send restored events #943
Conversation
@@ -89,6 +96,12 @@ export class SegmentDestination extends DestinationPlugin { | |||
configure(analytics: SegmentClient): void { | |||
super.configure(analytics); | |||
|
|||
// If the client has a proxy we don't need to await for settings apiHost, we can send events directly | |||
// Important! If new settings are required in the future you probably want to change this! |
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.
Should this maybe be a config option is we might need to change it in the future?
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.
A config in the client? I would prefer us to control it. If we end up changing it is because some code was added in the library itself to use those new settings that require awaiting. For people on old versions of the lib where this behavior is still around it shouldn't be a problem.
@@ -232,3 +232,23 @@ export function deepCompare<T>(a: T, b: T): boolean { | |||
|
|||
return true; | |||
} | |||
|
|||
export const createPromise = <T>( |
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.
Maybe a more descriptive name like createTimerPromise
or createTimeoutPromise
?
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.
it used to be createTimeoutPromise
but then I realized I made the timeout optional.
🎉 This PR is included in version 1.2.1 🎉 The release is available on npm package (@latest dist-tag) Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 0.4.2 🎉 The release is available on npm package (@latest dist-tag) Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 1.3.2 🎉 The release is available on npm package (@latest dist-tag) Your semantic-release bot 📦🚀 |
Problem reported that StartupFlushPolicy wouldn't send the pending upload events from the previous app launch.
Caused by concurrency: FlushPolicies get initialized before and without awaiting for storage restoration. Futhermore the Queue for upload is managed by the SegmentDestination plugin so there's no explicit way to await for that queue to be restored from the base client itself.
Solution:
shouldFlush
outside ofstart
(this is to be able to handle it with themanualFlush
)QueueFlushingPlugin
andSegmentDestination
to await for queue restoration and settings load when a flush is triggered. This guarantees that if the flush is triggered before the plugins are fully initialized (previous session events loaded) it will await for those operations to complete before uploading any events.