refactor(flags): Unify method to compute flag value and payload#2196
Merged
refactor(flags): Unify method to compute flag value and payload#2196
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Greptile Summary
This PR refactors the feature flags system in posthog-node by consolidating two separate methods (computeFlagLocally and computeFeatureFlagPayloadLocally) into a unified computeFlagAndPayloadLocally method. The refactoring addresses a fundamental design issue where maintaining separate code paths for flag value evaluation and payload retrieval was creating maintenance overhead and potential inconsistencies.
The new unified method accepts an optional matchValue parameter, allowing it to skip flag evaluation when a value is already known (useful for dependency-aware flag evaluation) while still computing the correct payload. Two new private helper methods were extracted: computeFlagValueLocally handles the core flag evaluation logic, and getFeatureFlagPayload manages payload retrieval with improved JSON parsing capabilities.
In client.ts, the getFeatureFlagPayload method was updated to use the new unified approach, including proper flag loading with await this.featureFlagsPoller?.loadFeatureFlags() and flag existence validation before computation. This change prepares the codebase for more sophisticated flag evaluation patterns like dependency chains while following the DRY principle and reducing cognitive load for developers working with the feature flags system.
Confidence score: 4/5
- This PR is safe to merge with minimal risk as it's a well-structured refactoring that maintains backward compatibility
- Score reflects solid code organization and clear separation of concerns, with only minor code quality issues identified
- Pay close attention to the duplicate default value assignment in
client.tslines 461-463 and the stray comment marker on line 450
2 files reviewed, 1 comment
Contributor
Contributor
|
Size Change: +4.52 kB (+0.1%) Total Size: 4.74 MB
ℹ️ View Unchanged
|
andyzzhao
approved these changes
Aug 6, 2025
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the feature flag evaluation system in the Node.js package by consolidating two separate methods (computeFlagLocally and computeFeatureFlagPayloadLocally) into a single unified method that computes both flag values and payloads together.
- Introduces a new
computeFlagAndPayloadLocallymethod that handles both flag evaluation and payload computation - Updates client code to use the unified method for more consistent evaluation paths
- Removes duplicate code paths that were previously causing confusion and maintenance issues
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/node/src/extensions/feature-flags/feature-flags.ts | Replaces separate flag and payload computation methods with unified approach, adds payload extraction logic |
| packages/node/src/client.ts | Updates payload retrieval to use the new unified method instead of separate evaluation calls |
Two separate methods for what is essentially the same operation is going to cause headaches down the line. This refactors both `compute*Locally` methods into a single one.
haacked
force-pushed
the
haacked/compute-locally-refactoring
branch
from
3e50a15 to
6ff7082
Compare
Add explicit checks for null and undefined values in getFeatureFlagPayload to prevent invalid payload lookups. This makes the logic more explicit and matches the behavior of the original computeFeatureFlagPayloadLocally method which only processed boolean and string values.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This refactors both
compute*Locallymethods into a single one.Problem
Two separate methods for what is essentially the same operation is going to cause headaches down the line.
For example, while working on local evaluation support for flag dependencies, I ended up having two code paths for dependency evaluation. First to evaluate the flag value in a dependency-aware manner. Then I had to evaluate flag payloads. This was confusing and brittle.
Changes
computeFlagAndPayloadLocallymethod that computes the flag value and payloadmatchValueand skips flag value evaluation if provided and uses that value to get the payload.client.tsto use the new method.computeFeatureFlagPayloadLocallyandcomputeFlagLocally.Release info Sub-libraries affected
Libraries affected
Checklist
If releasing new changes
This change doesn't need to be released right away.
pnpm changesetto generate a changeset file