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

fix(flags): Enqueue follow up requests without dropping #797

Merged
merged 4 commits into from
Sep 14, 2023

Conversation

neilkakkar
Copy link
Collaborator

@neilkakkar neilkakkar commented Sep 14, 2023

Changes

My previous PR delayed reloading to the end of the decide request: #791 , and for some reason I copied the wrong queue deletion behaviour instead of fixing it right in that PR 🤦 .

If posthog.identify() was called while we were requesting flags, we should queue this up and not drop it, as otherwise flags will stick around for the anon user rather than the identified one.

Fixes this behaviour, and removes the now-unused resetRequestQueue function

...

Checklist

@neilkakkar neilkakkar added the bump patch Bump patch version when this PR gets merged label Sep 14, 2023
Copy link
Contributor

@tiina303 tiina303 left a comment

Choose a reason for hiding this comment

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

🤷‍♀️

@github-actions
Copy link

github-actions bot commented Sep 14, 2023

Size Change: +160 B (0%)

Total Size: 685 kB

Filename Size Change
dist/array.full.js 177 kB +40 B (0%)
dist/array.js 118 kB +40 B (0%)
dist/es.js 118 kB +40 B (0%)
dist/module.js 118 kB +40 B (0%)
ℹ️ View Unchanged
Filename Size
dist/recorder-v2.js 95 kB
dist/recorder.js 58.3 kB

compressed-size-action

Copy link
Collaborator

@benjackwhite benjackwhite left a comment

Choose a reason for hiding this comment

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

Low context but tests look good so trusting you know what you're doing 😅

@neilkakkar
Copy link
Collaborator Author

Ok, figured out what the resetQueue was for: it's for any identify() that happens in the loaded callback, because that is called before decide is called, so we don't want to enqueue requests again for this.

Phew, this is very spider webby. But I added a few new explicit tests for this, to make future refactors easier.

@neilkakkar neilkakkar merged commit e3a296e into master Sep 14, 2023
12 checks passed
@neilkakkar neilkakkar deleted the fix-decide-pausing branch September 14, 2023 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump patch Bump patch version when this PR gets merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants