Skip to content

fix: Use strict typing in EvaluationEngine and fix evaluation boolean handling #36

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

Merged
merged 5 commits into from
May 29, 2025

Conversation

tyiuhc
Copy link
Collaborator

@tyiuhc tyiuhc commented May 29, 2025

Issue for reference.

Changes:

  1. Update EvaluationEngine to use strict typing
  2. Update EvaluationEngine to properly handle different boolean representations

Copy link

promptless bot commented May 29, 2025

✅ No documentation updates required.

@tyiuhc tyiuhc merged commit b2c5621 into main May 29, 2025
7 checks passed
github-actions bot pushed a commit that referenced this pull request May 29, 2025
## [1.2.3](1.2.2...1.2.3) (2025-05-29)

### Bug Fixes

* Use strict typing in EvaluationEngine and fix evaluation boolean handling ([#36](#36)) ([b2c5621](b2c5621))
Copy link

🎉 This PR is included in version 1.2.3 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@ruudk
Copy link
Contributor

ruudk commented Jun 3, 2025

This breaks the old behavior where you had to use "1" or "0" for it to work.
So this fix is a breaking change 🚨

When I deployed this our feature flags stopped working.

I think booleans should still work with 1 or 0.

@tyiuhc
Copy link
Collaborator Author

tyiuhc commented Jun 3, 2025

@ruudk, thanks for raising this. To clarify, are you using "1" and "0" in your flag definition to support boolean matching?

@ruudk
Copy link
Contributor

ruudk commented Jun 3, 2025

Yes and in PHP I feed a real Boolean. The old situation would convert the bool to "1" and thus it would match.

With your fix you broke that, and require me to match against true in amplitude. Fine, but others will get hit by this !

@tyiuhc
Copy link
Collaborator Author

tyiuhc commented Jun 3, 2025

@ruudk thank you for the clarification. To ensure the results of local and remote evaluation are the same, we recommend that existing flag configs using 1 and 0 for boolean targeting are updated to use true and false. Apologies for the unexpected behavior this has caused.

@ruudk
Copy link
Contributor

ruudk commented Jun 4, 2025

Sure, I can change my Feature Flag conditions in Amplitude. But my point is, that you broke existing behavior in a patch version. You should be more careful next time. Especially with something important as what you're offering. People rely on the feature flags to keep on working, after a patch release. Your answer doesn't give me confidence for future updates tbh.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants