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.
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
feat: adds RequireFlagsEnabled decorator #1159
base: main
Are you sure you want to change the base?
feat: adds RequireFlagsEnabled decorator #1159
Changes from all commits
b06ca5d
5e7dabe
eb3eb1f
c2c5157
bef054a
ce9852b
f056ae6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also we could have a context factory here too. Basically what we globally have for the OpenFeatureModule:
js-sdk/packages/nest/src/open-feature.module.ts
Line 138 in cf89e7d
But this would be optional to me. We do not have it for the other decorators too:
js-sdk/packages/nest/src/feature.decorator.ts
Line 36 in cf89e7d
So to me this is optional for this PR but it would be a great addition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, will do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, are you still planning to do it @kaushalkapasi? I can not see it yet, but you re-requested a review.
We can definitely leave it out for now if you want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lukas-reining I decided not to add this in for now.
Would you be able to elaborate how this would be used and how it would work differently compared to the ability to define the
context
(as on line 39)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries!
The difference is, that the factory could use the
ExecutionContext
to get request information like headers or IP address and transform that to evaluation context.This can currently only be done in the transaction context, having it here would allow e.g. to use a specific http header as context value for the specified flags only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there could be value in similar function like:
RequireFlagEquals
.But this could also be added in another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah! I opted to implement this first as the "easiest" option, a follow up would be to take a flag key and expected value combination to support String, Number and Object type flags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep that makes sense to me. I think most people would use this current decorator, which could basically just be a shorthand for a
RequireFlagEquals
implementation. I think instead of a key/value combination though, a lambda predicate might be easier (a lambda taking the EvalutionDetails which will run to decide to run the request or not). ForRequireFlagsEnabled
, the lambda would just beevalutationDetails.value === true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kaushalkapasi do you want to do this in this PR?
Because you requested reviews again but I could not find it yet.
We can also leave it out of here, but it might be really good to have here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't added the lambda function in this PR. My only concern is that it would complicate the usage of this Boolean only provider.
I would definitely add it when we build a decorator for other flag types (String, Number and Object).
Let me know if you think it should be implemented here and I'd be happy to do so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you do it like @toddbaert describes,
RequireFlagsEnabled
would simply be an alias forRequireFlagEquals
withevalutationDetails.value === true
.It would not make the
RequireFlagsEnabled
more complicated in this case.I would prefer to have it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, I'll update this implementation soon and tag you for a re-review when ready!