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

[Discuss] [Time To Visualize] Embeddable Responsiveness #84257

Closed
ThomThomson opened this issue Nov 24, 2020 · 1 comment
Closed

[Discuss] [Time To Visualize] Embeddable Responsiveness #84257

ThomThomson opened this issue Nov 24, 2020 · 1 comment

Comments

@ThomThomson
Copy link
Contributor

In introducing the Time to Visualize By Value / By Reference changes, I have run into this question a couple times.

Which types of input changes should embeddables be responsive to?

The Old Ways

In the old paradigm, embeddable factories would initialize embeddable input, and output at creation time. Most of their output would be static after creation, and they wouldn't properly update on change of many pieces of their input, like savedObjectId for instance, because this is only handled in the factories.

Why Were the Old Ways Lost?

Due to the async nature of unwrapping attributes that can either be by value or by reference, and the ability to change an embeddable from by reference -> by value and back again, it became necessary to shift much of the embeddable initialization logic into the embeddable itself.

Problems this has caused

  1. Because of the newly async nature of loading, embeddables need to be able to catch fatal errors any time in their lifecycle, not just during initialization: [Time to Visualize] Embeddable Error Handling Without ReplacePanel #82201

  2. Unlink / Add actions aren't reflected properly in embeddable outputs unless the embeddable is completely destroyed and rebuilt. [Time to Visualize] Unlink from Library does not properly delete savedObjectId #83864

Moving Forward

I see two solutions to move forward with this:

  1. We can move forward as is and not expect embeddables to be able to handle all types of input updates. This means that Time to Visualize and future projects that can change embeddable input in major ways will also have to ensure that they properly delete and re-create the changed embeddables every time. There is a draft PR [Time to Visualize] Fix Unlink Action via Rollback of ReplacePanel #83873 in place which applies this process to the add to library and unlink from library actions.

  2. This Pr [Dashboard] Fix cloning panels reactive issue #74253 has given us the possibility to take Embeddables in a new direction, in which any update to the input of an embeddable can be handled without fully deleting the embeddable, generating a new id, and creating the new embeddable from scratch. This would bring our embeddable implementation more in line with React Components, and would be much easier to understand in the future. As a simple POC I have implemented this change in Lens [Time to Visualize] Fix Unlink Action via Responsive Embeddables #83874.

Suggestion

I suggest that we implement option 1 for the time being, but also transition Lens, Maps, and Visualize to be able to handle changes to their savedObjectIds on the fly, and we recommend that any embeddables which need to support by value / by reference in the future do the same.

@ThomThomson
Copy link
Contributor Author

Closing this because this issue has been mostly solved!

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

No branches or pull requests

1 participant