fix(): prevent person processing if /decide fails to fetch remote config #1658
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Size Change: -330 B (-0.01%) Total Size: 3.25 MB
ℹ️ View Unchanged
|
havenbarnes
commented
Jan 18, 2025
| @@ -35,7 +35,7 @@ export const isObject = (x: unknown): x is Record<string, any> => { | |||
| // eslint-disable-next-line posthog-js/no-direct-object-check | |||
| return x === Object(x) && !isArray(x) | |||
| } | |||
| export const isEmptyObject = (x: unknown): x is Record<string, any> => { | |||
There was a problem hiding this comment.
This is a bit of an inaccurate use of the is keyword; my new usage of it in posthog-core.ts gets mad because a false return from isEmptyObject tells TS that this.config is "not an object" when it could instead just be "not an empty object"
havenbarnes
changed the title
dmarticus
approved these changes
Jan 21, 2025
havenbarnes
requested review from
ioannisj and
marandaneto
and removed request for
haacked and
benjackwhite
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
Context:
In digging through the code it appears that there are two cases where this could potentially happen:
_initsettingthis.config = {}(here) and it getting re-set to the proper value (this seems like an extremely unlikely race condition but we may as well protect against it in_hasPersonProcessing, and while writing unit tests for a potential fix here I discovered that all events would fail to send ifposthog.configwas an empty object anyway)/decidefails to return the config values, leaving the SDK to fall back toperson_profiles: 'always'(here)To protect against the second edge case we make the following change: Remove the dependency on the
defaultIdentifiedOnlyfield from/decide. This is the new default, so we should not need this conditional moving forward anyway.Checklist