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

feat(flags): Allow adding person and group property overrides for flags #613

Merged
merged 8 commits into from
Apr 26, 2023

Conversation

neilkakkar
Copy link
Collaborator

@neilkakkar neilkakkar commented Apr 18, 2023

Changes

When you want properties to be reflected immediately for feature flags, you can now use these functions to override server properties & not wait for ingestion to resolve flags.

Docs here: PostHog/posthog.com#5827

...

Checklist

@neilkakkar neilkakkar requested a review from EDsCODE April 18, 2023 13:30
@github-actions
Copy link

github-actions bot commented Apr 18, 2023

Size Change: +7.08 kB (0%)

Total Size: 2.3 MB

Filename Size Change
dist/array.full.js 156 kB +1.77 kB (+1%)
dist/array.js 97.8 kB +1.77 kB (+2%)
dist/es.js 97.6 kB +1.77 kB (+2%)
dist/module.js 97.9 kB +1.77 kB (+2%)
ℹ️ View Unchanged
Filename Size
dist/recorder-v2.js 93.6 kB
dist/recorder.js 58.3 kB
lib/rrweb/dist/plugins/console-record.js 26.6 kB
lib/rrweb/dist/plugins/console-record.min.js 10.8 kB
lib/rrweb/dist/plugins/console-replay.js 12.7 kB
lib/rrweb/dist/plugins/console-replay.min.js 5.72 kB
lib/rrweb/dist/plugins/sequential-id-record.js 910 B
lib/rrweb/dist/plugins/sequential-id-record.min.js 417 B
lib/rrweb/dist/plugins/sequential-id-replay.js 1.24 kB
lib/rrweb/dist/plugins/sequential-id-replay.min.js 578 B
lib/rrweb/dist/record/rrweb-record-pack.js 31.4 kB
lib/rrweb/dist/record/rrweb-record-pack.min.js 10.7 kB
lib/rrweb/dist/record/rrweb-record.js 140 kB
lib/rrweb/dist/record/rrweb-record.min.js 51.7 kB
lib/rrweb/dist/replay/rrweb-replay-unpack.js 188 kB
lib/rrweb/dist/replay/rrweb-replay-unpack.min.js 71.1 kB
lib/rrweb/dist/replay/rrweb-replay.js 172 kB
lib/rrweb/dist/replay/rrweb-replay.min.js 66.4 kB
lib/rrweb/dist/rrweb-all.js 356 kB
lib/rrweb/dist/rrweb-all.min.js 133 kB
lib/rrweb/dist/rrweb.js 302 kB
lib/rrweb/dist/rrweb.min.js 116 kB
lib/rrweb/es/rrweb/ext/base64-arraybuffer/dist/base64-arraybuffer.es5.js 1.94 kB
lib/rrweb/es/rrweb/ext/mitt/dist/mitt.es.js 1.99 kB

compressed-size-action

@neilkakkar neilkakkar marked this pull request as ready for review April 21, 2023 12:54
Copy link
Member

@EDsCODE EDsCODE left a comment

Choose a reason for hiding this comment

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

lgtm besides naming

* This is used when dealing with new persons / where you don't want to wait for ingestion
* to update user properties.
*/
personPropertiesForFlags(properties: Properties, reloadFeatureFlags = true): void {
Copy link
Member

Choose a reason for hiding this comment

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

Naming on these functions could be better. They sound like getters

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

setPersonPropertiesForFlags ?

Copy link
Member

Choose a reason for hiding this comment

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

yep

@EDsCODE
Copy link
Member

EDsCODE commented Apr 24, 2023

One extra thought: would it not be better to have a general "override person properties" function that does the same thing just makes it more clear you're overriding person/group properties for this person/group in focus. Or maybe even just do this implicitly if user/group properties are ever set.

Just trying to think how to make this clearer because all the overridability makes the experience pretty confusing. Now there's more concepts—bootstrapping, local evaluation, and person/group overrides

@neilkakkar
Copy link
Collaborator Author

neilkakkar commented Apr 25, 2023

Not sure what you mean by 'this person/group in focus'. Isn't it already doing that? It's implicitly adding props for current person/group, when they're passed in.

Just trying to think how to make this clearer because all the overridability makes the experience pretty confusing. Now there's more concepts—bootstrapping, local evaluation, and person/group overrides

Agree. On the bright-ish side, most people shouldn't have to use this - it works out of the box, it's only for some special use-cases, where you're not setting person properties but want them to be used for flags, that this comes into the picture.

It's somewhat like local evaluation, but in client-side SDKs, where you're again passing in properties to evaluate.

@neilkakkar neilkakkar merged commit c28d1a1 into master Apr 26, 2023
10 checks passed
@neilkakkar neilkakkar deleted the person-props branch April 26, 2023 08:47
@neilkakkar neilkakkar mentioned this pull request Apr 26, 2023
2 tasks
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.

None yet

2 participants