[Embeddables Rebuild] Migrate Visualize #183197
Conversation
…e-react-converstion
…e-react-converstion
…e-react-converstion # Conflicts: # src/plugins/visualizations/public/plugin.ts
Zacqary
added
Team:Presentation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
|
Pinging @elastic/kibana-presentation (Team:Presentation) |
|
@elasticmachine merge upstream |
| const observer = new MutationObserver((mutationList) => { | ||
| for (const mutation of mutationList) { | ||
| if (mutation.type === 'attributes') { | ||
| if ( | ||
| mutation.attributeName === 'data-render-complete' && | ||
| visualization.getAttribute('data-render-complete') === 'true' | ||
| ) { | ||
| observer.disconnect(); | ||
| resolve(); | ||
| } | ||
| } | ||
| } | ||
| }); | ||
| observer.observe(visualization, { attributes: true }); |
There was a problem hiding this comment.
I didn't run this locally, but I suppose we are leveraging mutation observers because the renderComplete event no longer gets dispatched, I see that it ideally would have been dispatched here https://github.com/elastic/kibana/blob/main/src/plugins/visualizations/public/embeddable/visualize_embeddable.tsx#L392, is this intentional?
There was a problem hiding this comment.
Yes, in the new embeddable framework everything goes through React, so it's much more cumbersome to get the kind of DOM access we need to emit events from the correct element. We're trying to move away from event emitters in new code.
There was a problem hiding this comment.
My concern is that previously the implementation detail for when a render completes was contained, and any interested consumer of visualizations need only care if a renderComplete event was emitted on any said visualization element. I think that a replacement util for signalling renderComplete in the new code should be provided, as it rather seems that the implementation detail of determining if a render has completed is now being shifted on to the consumer.
| VISUALIZE_EMBEDDABLE_TYPE, | ||
| VisualizeEmbeddableFactory, | ||
| } from './embeddable'; | ||
| import { createVisEmbeddableFromObject } from './embeddable'; |
There was a problem hiding this comment.
Can you async import createVisEmbeddableFromObject where its used in mount callback?
| vis$.subscribe((vis) => vis.uiState.on('change', onUiStateChange)); | ||
|
|
||
| // When the serialized vis changes, update the vis instance | ||
| serializedVis$.subscribe(async (serializedVis) => { |
There was a problem hiding this comment.
This should use a switchMap to fetch async items. Then subscribe should apply them. Otherwise, you are introducing a race condition where an serializedVis$ emit before the previous serializedVis$ emit async tasks complete may call .next out of order.
serializedVis$.pipe(
switchMap((serializedVis) => {
const vis = await createVisInstance(serializedVis)
const const { params, abortController } = await getExpressionParams();
return { vis, params, abortController };
})
).subscribe((vis, params, abortController) => {
vis$.next(vis);
if (params) expressionParams$.next(params);
expressionAbortController$.next(abortController);
})
There was a problem hiding this comment.
Looks like you lost the below in the fix. Is that still needed?
const currentVis = vis$.getValue();
if (currentVis) currentVis.uiState.off('change', onUiStateChange);
There was a problem hiding this comment.
Probably necessary yeah, might have memory leaks without
|
/ci |
|
@elasticmachine merge upstream |
|
I guess that doesn't work anymore? |
|
/ci |
| onAdd: (container, savedObject) => { | ||
| container.addNewPanel<VisualizeRuntimeState>({ | ||
| panelType: VISUALIZE_EMBEDDABLE_TYPE, | ||
| initialState: savedObjectToRuntimeState(savedObject), |
There was a problem hiding this comment.
Shouldn't container.addNewPanel get passed serializedState? Why are you converting the state to runtime state? What happens if you remove savedObjectToRuntimeState?
The reason I ask is that this PR adds 10KB to the page load size and am trying to track down all possible increases. It would be great if we could avoid importing savedObjectToRuntimeState into the page load bundle size. I don't think this is needed.
There was a problem hiding this comment.
Just tested it and the following works.
container.addNewPanel<VisualizeSerializedState>({
panelType: VISUALIZE_EMBEDDABLE_TYPE,
initialState: { savedObjectId: savedObject.id },
});
There was a problem hiding this comment.
At one point this was necessary after a merge with main broke the embeddable, but I guess something with the panel handling changed and now it's no longer necessary? Still works without passing this in. I'll remove.
|
/ci |
| if (hasRendered) { | ||
| domNode.current?.dispatchEvent(new Event('renderComplete')); | ||
| } | ||
| }, [hasRendered]); |
There was a problem hiding this comment.
Thanks for opting to support signalling render completion using events. I wonder though if we could leverage the already existing util for doing this in place for constructing the event inline. See here
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
async chunk count
References to deprecated APIs
History
To update your PR or re-run it, just comment with: |
This reverts commit ee28e20.
Summary
Closes #174958
This migrates the Visualize embeddable to the new React Embeddable framework.
Migrated:
Also adds deprecation statements to legacy Embeddable factories
In a second PR, we'll move the
embeddablefolder tolegacy/embeddableand renamereact_embeddabletoembeddable. I don't know if git will be able to diff that change in a comprehensible way in this PR, so I want to save it for the next one.Checklist