Add basic telemetry support - #36
Conversation
Sorry, something went wrong.
| } | ||
|
|
||
| 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?
Sorry, something went wrong.
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
Sorry, something went wrong.
| 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?
Sorry, something went wrong.
There was a problem hiding this comment.
I followed the pattern used by Yan Zhang (@Eskibear) https://github.com/microsoft/FeatureManagement-JavaScript/blob/main/test/featureManager.test.ts
Sorry, something went wrong.
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)
Sorry, something went wrong.
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
Sorry, something went wrong.
Ross Grambo (rossgrambo)
left a comment
There was a problem hiding this comment.
Have you checked parity with dotnet/python? Using the https://github.com/microsoft/FeatureManagement samples
Sorry, something went wrong.
|
Ross Grambo (@rossgrambo) Yan used these feature flags for test which are from the centralized feature flag sample. The targeting hash behavior was guarded by it. |
Sorry, something went wrong.
Why this PR?
Allow user to set onFeatureEvaluated callback which be executed whenever a feature flag with telemetry enabled is evaluated.