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.

Sign up for GitHub

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

Jump to bottom

[Embeddables Rebuild] Fix runtime state types. #186194

Merged
merged 5 commits into from
Jun 19, 2024
Merged

[Embeddables Rebuild] Fix runtime state types. #186194

merged 5 commits into from
Jun 19, 2024

Conversation

ThomThomson
Copy link
Contributor

@ThomThomson ThomThomson commented Jun 13, 2024 •
edited
Loading

Summary

While working on #186052 I noticed that the snapshotRuntimeState interface was being passed serializedState instead of runtimeState which made for some awkward casts and was causing type errors in the PR.

I split those changes out into this PR because they unfortunately required me to change the order of the type arguments to the embeddable factory, hence the large number of changes and number of teams pinged. Changes should be type only, but if your Embeddable has no difference between its serialized and runtime state, you'll notice the serialized state type is specified twice to make this explicit.

@ThomThomson
Copy link
Contributor Author

/ci

@ThomThomson
Copy link
Contributor Author

/ci

@ThomThomson ThomThomson added Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas loe:small Small Level of Effort release_note:skip Skip the PR/issue when compiling release notes impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. Feature:Embeddables Relating to the Embeddable system labels Jun 13, 2024
@ThomThomson ThomThomson marked this pull request as ready for review June 13, 2024 20:02
@ThomThomson ThomThomson requested review from a team as code owners June 13, 2024 20:02
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

qn895
qn895 approved these changes Jun 13, 2024
View reviewed changes
Copy link
Member

@qn895 qn895 left a comment

Choose a reason for hiding this comment

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

ML changes LGTM

@botelastic botelastic bot added ci:project-deploy-observability Create an Observability project Feature:Embedding Embedding content via iFrame Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team labels Jun 13, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services)

nickpeihl
nickpeihl approved these changes Jun 17, 2024
View reviewed changes
Copy link
Member

@nickpeihl nickpeihl left a comment

Choose a reason for hiding this comment

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

type changes lgtm! added a suggestion, but nothing blocking.

examples/embeddable_examples/public/react_embeddables/field_list/types.ts
export type FieldListApi = DefaultEmbeddableApi & PublishesSelectedFields & PublishesDataViews;
export type FieldListApi = DefaultEmbeddableApi<
FieldListSerializedStateState,
FieldListSerializedStateState
Copy link
Member

Choose a reason for hiding this comment

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

Nit: The RuntimeState type matches the SerializedState type, which makes sense to me. But I wonder if, for the examples, we should specifically add a RuntimeState type like type FieldListRuntimeState = FieldListSerializedStateState to make it clearer for engineers jumping into the new framework?

Or maybe just a comment explaining that SerializedState and RuntimeState are equivalent?

Copy link
Contributor

Choose a reason for hiding this comment

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

we could also add RuntimeState and SerializedState to embeddable plugin readme.md to help explain why the 2 versions of state exist and an example (writen, not code) of when they should be different

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nickpeihl I think that is a good idea. I've seen that pattern used in quite a few of the embeddables made by other teams too.

I only wish there was a way to default it to the serialized state and allow implementors to skip adding it - but that isn't possible because of the order of the template.

@nreese I think the documentation is also a good idea, but if it's okay I'll skip that for this PR so that we can get it in ASAP to unblock #186052

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this in ab4326e

@ThomThomson
Copy link
Contributor Author

@elasticmachine merge upstream

rshen91
rshen91 approved these changes Jun 17, 2024
View reviewed changes
Copy link
Contributor

@rshen91 rshen91 left a comment

Choose a reason for hiding this comment

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

Shared UX changes LGTM

@ThomThomson ThomThomson requested a review from a team as a code owner June 17, 2024 19:40
@kibana-ci
Copy link
Collaborator

kibana-ci commented Jun 17, 2024 •
edited
Loading

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

✅ unchanged

History

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

@mgiota mgiota self-requested a review June 18, 2024 18:36
mgiota
mgiota approved these changes Jun 18, 2024
View reviewed changes
Copy link
Contributor

@mgiota mgiota left a comment

Choose a reason for hiding this comment

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

Type changes LGTM! I agree with @nickpeihl 's suggestion, but I approve this PR to unblock it.

jennypavlova
jennypavlova approved these changes Jun 19, 2024
View reviewed changes
Copy link
Member

@jennypavlova jennypavlova left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@ThomThomson ThomThomson merged commit 6d39b8a into elastic:main Jun 19, 2024
25 checks passed
@kibanamachine kibanamachine added v8.15.0 backport:skip This commit does not require backporting labels Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nreese nreese nreese left review comments

@Kerry350 Kerry350 Kerry350 approved these changes

@nickpeihl nickpeihl nickpeihl approved these changes

@mgiota mgiota mgiota approved these changes

@Heenawter Heenawter Heenawter approved these changes

@jennypavlova jennypavlova jennypavlova approved these changes

@rshen91 rshen91 rshen91 approved these changes

@qn895 qn895 qn895 approved these changes

Assignees
No one assigned
Labels
apm:review backport:skip This commit does not require backporting ci:project-deploy-observability Create an Observability project Feature:Embeddables Relating to the Embeddable system Feature:Embedding Embedding content via iFrame impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:small Small Level of Effort release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.15.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.