Add basic telemetry support #36
Conversation
zhiyuanliang-ms
requested review from
Eskibear,
linglingye001 and
rossgrambo
| } | ||
|
|
||
| interface FeatureManagerOptions { | ||
| export interface FeatureManagerOptions { |
There was a problem hiding this comment.
what's the purpose of code move? to put exported types together? Is there any change other than that?
There was a problem hiding this comment.
public stuff should be placed in front of private stuff. I follow this practice in .NET
Is there any change other than that?
No
| import * as chai from "chai"; | ||
| const expect = chai.expect; | ||
|
|
||
| import { FeatureManager, ConfigurationObjectFeatureFlagProvider, EvaluationResult, VariantAssignmentReason } from "../"; |
There was a problem hiding this comment.
Can you put import statements together at the beginning of the file?
There was a problem hiding this comment.
I followed the pattern used by @Eskibear https://github.com/microsoft/FeatureManagement-JavaScript/blob/main/test/featureManager.test.ts
There was a problem hiding this comment.
But I found that the testsuite in JS provider, put all import statements in the beginning. I will make them consistent. But personally, I like the way in this repo.
Recall that we've had this discussion before: #24 (comment)
There was a problem hiding this comment.
I prefer the imports together. In the /example code- it might be helpful to keep them apart to clearly show the reader which parts relate to what. But I don't think it's valuable outside of an example
There was a problem hiding this comment.
Have you checked parity with dotnet/python? Using the https://github.com/microsoft/FeatureManagement samples
|
@rossgrambo Yan used these feature flags for test which are from the centralized feature flag sample. The targeting hash behavior was guarded by it. |
Why this PR?
Allow user to set onFeatureEvaluated callback which be executed whenever a feature flag with telemetry enabled is evaluated.