-
Notifications
You must be signed in to change notification settings - Fork 44
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: Move ready queue processing after identity request #933
base: development
Are you sure you want to change the base?
fix: Move ready queue processing after identity request #933
Conversation
8b5ffb4
to
6478269
Compare
src/mp-instance.js
Outdated
// If for some reason the identity call inside of Session Manager does not run | ||
// we should clear out the ready queue. |
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.
Your PR description appropriately states when this is the case, rather than "for some reason", so I'd turn that into a comment.
"Will also continue to clear out the ready queue as part of the initial init flow if an identify request is unnecessary, such as if there is an existing session".
const functionSpy = jest.fn(); | ||
(window.mParticle as any) = { | ||
fakeFunction: functionSpy, | ||
args: () => {}, |
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.
Is this args
function necessary? I haven't pulled this code down, but as I read through the processReadyQueue
function, the only times I see args
is where it references the item in the array, as opposed to it being a key on window.mParticle
.
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.
I'm open to discussing this further. Adding args
here was the only way I could get it to hit that specific codepath in my tests.
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.
let's hop on a zoom about this to walk through
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.
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.
one last comment, but looks good otherwise
Quality Gate failedFailed conditions |
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.
lgtm!
Instructions
development
Summary
Testing Plan
Reference Issue (For mParticle employees only. Ignore if you are an outside contributor)