-
Notifications
You must be signed in to change notification settings - Fork 43
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
Fix(flags): False positive when variant rule is an async function #876
base: main
Are you sure you want to change the base?
Conversation
Tagging OptionsShould a new tag be published when this PR is merged?
|
@@ -64,7 +65,7 @@ const flagBlock: Block<BlockModule<FlagFunc>> = { | |||
>(func: { | |||
default: FlagFunc<TConfig>; | |||
}) => | |||
($live: TConfig, { request }: HttpContext) => { | |||
async ($live: TConfig, { request }: HttpContext) => { |
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.
this will impact tracings as only async functions are included on tracing. This shouldn't be though but we should keep an eye on it.
blocks/flag.ts
Outdated
// the rule can sometimes be a promise and we need to await it to check if it's truthy or not | ||
if (isAwaitable(ruleResult)) { | ||
try { | ||
ruleResult = await ruleResult; |
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.
in the case you provided (when site.ts has an app middleware) this code runs on serial (one await for each matcher) instead of parallel since all matchers will returns a promise, right? Given that, you should instead, parallelize the rule evaluation.
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.
not at all, matchers coming from "website" app continue as sync
functions, but my local ones, that depends on my async middleware, turns into async
function
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 mean, anyways introducing a middleware will cause all your matchers to be async, so it worth to make parallel instead, wdyt?
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.
make sense, your proposal is to always use Promise.all
on this list, right?
Promise.all(flags.variants.map((variant) => variant.rule(ctx)).find((result) => result)
There are scenarios where we have a multivariant (e.g Pages with variants) and
variant.rule
can return a Promise even if the matcher module returns a synchronous function, this occurs when I have a matcher in my project and an (asynchronous) middleware in my appsite.ts
, makingvariant.rule
asynchronous as well. It doesn't impact matchers that are from other apps likewebsite
app that have a lot of matchers, but only the local onesIllustration of my problem.
The
variant.rule
check is always true since Promise is a truly value, even if the final value of the promise is false.This PR makes it possible for us to detect asynchronous functions in the flags block, where the problem was occurring, and if necessary we make sure the promise is resolved so that we can check if the
variant.rule
returnstrue