Skip to content
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

Merged
merged 2 commits into from
Aug 11, 2021

Conversation

dokmic
Copy link
Contributor

@dokmic dokmic commented Aug 10, 2021

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 how concatMap works. The expressions execution services returns never completing observable, so the concatMap 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

pluck('result'),
merge(defer(() => args.default?.() ?? of(input))),
take(1)
const cases = args.case?.map((item) => defer(() => item())) ?? [];
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
canvas 1.6MB 1.6MB -212.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@dokmic dokmic marked this pull request as ready for review August 11, 2021 06:56
@dokmic dokmic requested a review from a team as a code owner August 11, 2021 06:56
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-services (Team:AppServices)

Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code LGTM

Copy link
Contributor

@clintandrewhall clintandrewhall left a 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).

@th0ger
Copy link

th0ger commented Sep 3, 2021

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.

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.

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

Successfully merging this pull request may close these issues.

Canvas Asset Rendering Only Shows Default Asset
7 participants