-
Notifications
You must be signed in to change notification settings - Fork 0
Replace errors with warnings in identify()
#115
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
Conversation
Replaces thrown errors with warnings for invalid API usage in `identify()` and `updateTraits()`. `identify()` now auto-resets when identifying a different user to prevent identity merging, and `updateTraits()` returns early instead of throwing when called without identification.
When identifying a different user on an already identified session:
- Before: Threw an error, blocking execution
- After: Warns and auto-resets the session, then proceeds with the new identification
```mermaid
flowchart TD
A[identify called] --> B{Already identified?}
B -->|No| C[Identify user]
B -->|Yes| D{Same user ID?}
D -->|Yes| C
D -->|No| E[Warn dev]
E --> F[Auto-reset session]
F --> C
```
When called without identifying a user first:
- Before: Threw an error, blocking execution
- After: Warns and returns early, allowing queued commands to continue processing
```mermaid
flowchart LR
A[updateTraits called] --> B{User identified?}
B -->|Yes| C[Update traits]
B -->|No| D[Warn dev]
D --> E[Return early]
E --> F[Continue queue processing]
```
| Behavior | PostHog | This Implementation | Rationale |
|----------|---------|---------------------|-----------|
| Throws error | ❌ No | ❌ No (warns instead) | ✅ Aligned — non-disruptive error handling |
| Auto-reset on different ID | ❌ No | ✅ Yes | ⚠️ More defensive — prevents identity merging |
| Warning message | ❌ No | ✅ Yes | ⚠️ Better DX — helps developers catch issues |
**Design decision**: Auto-reset prevents accidental identity merging, which can corrupt analytics data. PostHog silently updates the distinct ID and links identities, which can merge separate users unintentionally. This implementation prioritizes data integrity over silent behavior.
- Better development experience: invalid usage no longer crashes applications
- Queue processing: subsequent queued commands continue even if earlier ones fail validation
- Data integrity: auto-reset prevents accidental identity merging
- Backward compatible: API surface unchanged, only error handling behavior modified
Tests updated to verify warning behavior instead of thrown errors.
redox
left a comment
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.
This looks
packages/altertable-js/src/core.ts
Outdated
| `User "${userId}" is already identified as "${this._sessionManager.getDistinctId()}". This usually indicates a development issue, as it would merge two separate identities.\n\n` + | ||
| `The session has been automatically reset. Use alias() to link the new ID to the existing one if intentional.` |
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.
FWIW the backend wouldn't merge the 2 identities in this case as they're both identified: https://github.com/altertable-ai/backend/blob/69206d849ef32a230a5b8222d4bef771dd64008a/api/src/common/product_analytics/identities/merge.rs#L117-L120
I think it's a fair safeguard but maybe we should just log the warning and proceed (without early return).
@redox Wdyt?
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.
The early return has been removed, no? I think even if the API supports it, this reset makes sense
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.
Indeed, then I'd just remove this part of the warning: "as it would merge two separate identities"
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.
Sounds good — updated in 89dbae2.
Replaces thrown errors with warnings for invalid API usage in
identify()andupdateTraits().identify()now auto-resets when identifying a different user to prevent identity merging, andupdateTraits()returns early instead of throwing when called without identification.Details
identify()behavior changeWhen identifying a different user on an already identified session:
flowchart TD A[identify called] --> B{Already identified?} B -->|No| C[Identify user] B -->|Yes| D{Same user ID?} D -->|Yes| C D -->|No| E[Warn dev] E --> F[Auto-reset session] F --> CupdateTraits()behavior changeWhen called without identifying a user first:
flowchart LR A[updateTraits called] --> B{User identified?} B -->|Yes| C[Update traits] B -->|No| D[Warn dev] D --> E[Return early] E --> F[Continue queue processing]Comparison with PostHog
Design decision: Auto-reset prevents accidental identity merging, which can corrupt analytics data. PostHog silently updates the distinct ID and links identities, which can merge separate users unintentionally. This implementation prioritizes data integrity over silent behavior.
Impact
Tests updated to verify warning behavior instead of thrown errors.