Skip to content

Conversation

@Eskibear
Copy link
Member

@Eskibear Eskibear commented Aug 7, 2024

See #14
It validates property types based on the feature flag JSON schema.

zhiyuanliang-ms
zhiyuanliang-ms previously approved these changes Aug 8, 2024
@zhiyuanliang-ms
Copy link
Member

This PR will not close #14. After we support telemetry and variant, we need to validate their properties.

@zhiyuanliang-ms
Copy link
Member

@Eskibear @linglingye001 I am thinking whether the validation is too much. In our current implementation, the validation will happen during each call of isEnabled, this could be a performance bottle neck.

In .NET FM, the validation will only happen once, because feature flag provider has a cache mechanism (but we don't have that and I am also wondering whether it is possible to have that because we cannot know whether the configuration changed so that we can refresh the cache.)

So, I am thinking maybe we can add an option to disable the feature flag validation. As long as the feature flag is from app config, we should have the confidence that feature flags are in the correct shape.

zhiyuanliang-ms
zhiyuanliang-ms previously approved these changes Nov 5, 2024
@zhiyuanliang-ms
Copy link
Member

Except the comments I left, this PR looks good to me.

@Eskibear
Copy link
Member Author

Eskibear commented Nov 6, 2024

@zhiyuanliang-ms do you want me to update the PR or you take it over?

@zhiyuanliang-ms
Copy link
Member

@zhiyuanliang-ms do you want me to update the PR or you take it over?

@Eskibear I asked @linglingye001 to take over this PR. Did you notice that lingling has pushed a lot of commits?

zhiyuanliang-ms
zhiyuanliang-ms previously approved these changes Nov 6, 2024
@linglingye001 linglingye001 merged commit e7119e2 into main Nov 6, 2024
5 checks passed
@linglingye001 linglingye001 deleted the yanzh/ff-validation branch November 6, 2024 08:22
@vputsenko
Copy link

vputsenko commented Jan 8, 2025

See #74, you added bug in this PR.

@zhiyuanliang-ms
Copy link
Member

@vputsenko Sorry for the mistake. This is a bad bug. I didn't notice it during code review. That's my fault. We added this in 2.0.0-preview.3. Other packages before it should be fine. I will publish a bug fix release very soon. Sorry for the inconvenience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants