Skip to content
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

Pre-init queue for events #243

Merged
merged 4 commits into from
Mar 14, 2025
Merged

Pre-init queue for events #243

merged 4 commits into from
Mar 14, 2025

Conversation

evan-masseau
Copy link
Contributor

We have this pre-init queue specifically for push opened events, but we had the same comment on there for the standard createEvent method. I'm not sure whether the mistake is having the comment, or missing the preInitQueue. Given react native timing issues, i'm inclined to add the flexibility for any event creation though.

…ate events prior to initialize, but this method doesn't actually allow for it :oops:
@evan-masseau evan-masseau requested a review from a team as a code owner March 13, 2025 15:01
Copy link
Contributor

@dan-peluso dan-peluso left a comment

Choose a reason for hiding this comment

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

Are these the only two we'd want that on? I know the registry set stuff is pretty agnostic of having klaviyo be initialized, but for the sake of retrying and keeping errors visible I wonder if it'd hurt us to include this on most of our safe apply calls

@evan-masseau
Copy link
Contributor Author

@dan-peluso yeah thats what I'm kinda grappling with right now.
For background, when we first implemented this we had the philosophy that this should be the exception, not the rule. It was specifically a work around for the fact that in react native, opened push events are created from the push service code that runs prior to any react native code being able to run.
I put this PR up to think about it more... This activity issue got me thinking more about timing issues. I was also going to try to find slack threads from that earlier decision -- although this failing test has me thinking that we did intentionally decide to make this only an exception for push opens, since i explicitly wrote a test case that another event type wouldn't get created, and must have just forgotten to take out the comment.

@dan-peluso
Copy link
Contributor

@dan-peluso yeah thats what I'm kinda grappling with right now. For background, when we first implemented this we had the philosophy that this should be the exception, not the rule. It was specifically a work around for the fact that in react native, opened push events are created from the push service code that runs prior to any react native code being able to run. I put this PR up to think about it more... This activity issue got me thinking more about timing issues. I was also going to try to find slack threads from that earlier decision -- although this failing test has me thinking that we did intentionally decide to make this only an exception for push opens, since i explicitly wrote a test case that another event type wouldn't get created, and must have just forgotten to take out the comment.

Thanks for the context. It makes sense it's a specific RN exception. Are there any concerns with adding it to the other safe apply calls? I usually lean towards the side of 'if someone can do it, they will' so to me it seems like this could be a good way to make errors obvious to devs.

@evan-masseau
Copy link
Contributor Author

evan-masseau commented Mar 13, 2025

Thanks for the context. It makes sense it's a specific RN exception. Are there any concerns with adding it to the other safe apply calls? I usually lean towards the side of 'if someone can do it, they will' so to me it seems like this could be a good way to make errors obvious to devs.

@dan-peluso our concern was/is basically that if we open the door to making these calls when you haven't initialized, data loss ensues if someone takes that too far and queues up operations that, if the app closes before ever initializing, we will simply lose completely because we couldn't persist the data. The classic if we give you too much rope you might hang yourself thing -- pardon the macabre i couldn't think of another idiom right now.

Honestly as I re-visit the past reasoning, I think what I want this PR to be is to remove the comment from createEvent, as opposed to enabling all events to be created pre-initialize.

@evan-masseau evan-masseau merged commit def1a5b into master Mar 14, 2025
6 checks passed
@evan-masseau evan-masseau deleted the ecm/pre-init-events branch March 14, 2025 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants