-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Expressions] Add support of partial results to the switch expression function #108086
Conversation
pluck('result'), | ||
merge(defer(() => args.default?.() ?? of(input))), | ||
take(1) | ||
const cases = args.case?.map((item) => defer(() => item())) ?? []; |
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.
According to the Expressions spec above, case
is a required argument. I'm curious why the Argument interface was changed in #100409 and if/why it needs to remain so.
@ppisljar I thought the types I had written in Canvas would through a TS error if required
was true and the argument was optional... I may be misremembering, though.
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.
That's a good finding. I've corrected types because we had that check in the implementation and, for some reason, test cases for that. I remember @ppisljar asked me to correct all the optional types, but it seems like I forgot to correct this one here.
Please see my changes in the latest commit.
💚 Build Succeeded
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
Pinging @elastic/kibana-app-services (Team:AppServices) |
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.
code LGTM
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.
Code looks fine. What's our plan to communicate this bug and fix to users? We've gotten one or two bug reports so far, but I'm worried of more, given how integral switch
can be to some workpads.
This also escalates our need to relocate these functions out of Canvas, @ppisljar , (an old refrain now, I know).
You should really add this bug to release note of 7.14.0 and fix to release note of 7.14.1. Otherwise you will keep seeing duplicates like mine #110509 . Even worse people seing wrong data without knowing. |
Summary
This PR resolves #107934 by changing the
switch
function's implementation to handle observables on input correctly.In 7.14, there is a problem with the
switch
function caused by #100409 — the reason for the incorrect behavior is in the way howconcatMap
works. The expressions execution services returns never completing observable, so theconcatMap
stops awaiting first case completion.There is no such problem in 7.x and the mainstream branch because the behavior was changed by #102403. But even then, the switch function will incorrectly be handling expressions returning partial results.
Checklist
Delete any items that are not applicable to this PR.
For maintainers