-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Discover] Deangularization of search embeddable #100552
[Discover] Deangularization of search embeddable #100552
Conversation
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.
Tested in Chrome, Firefox, Safari, works as expected, LLGW! will have a closer look when you flag ready for review
src/plugins/discover/public/application/components/create_discover_grid_directive.tsx
Outdated
Show resolved
Hide resolved
src/plugins/discover/public/application/embeddable/saved_search_embeddable.tsx
Outdated
Show resolved
Hide resolved
@elasticmachine merge upstream |
Pinging @elastic/kibana-app (Team:KibanaApp) |
src/plugins/discover/public/application/angular/doc_table/create_doc_table_embeddable.tsx
Outdated
Show resolved
Hide resolved
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.
Review in progress, one more file, looks pretty nice, great work!
src/plugins/discover/public/application/angular/doc_table/create_doc_table_embeddable.tsx
Outdated
Show resolved
Hide resolved
src/plugins/discover/public/application/components/discover_grid/discover_grid.tsx
Outdated
Show resolved
Hide resolved
src/plugins/discover/public/application/embeddable/saved_search_embeddable.tsx
Outdated
Show resolved
Hide resolved
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 👍 , found 2 more files to cleanup: search_template.html, search_template_datagrid.html, will continue with checking performance
if (renderProps.refs) { | ||
const fn = getRenderFn(renderProps.refs, renderProps); | ||
fn().then((newScope) => { | ||
scope.current = newScope; | ||
}); | ||
} |
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.
I think I've found the reason why the domNode count is much higher in this PR compared to the one in master. So currently, with the current code, the classic table is re-rendered completely with every change. with the following change, it would just render once, and then Angular would take care of the additional renderings:
if (renderProps.refs && !scope.current) {
const fn = getRenderFn(renderProps.refs, renderProps);
fn().then((newScope) => {
scope.current = newScope;
});
} else if (scope && scope.current) {
scope.current.renderProps = { ...renderProps };
scope.current.$applyAsync();
}
Some additional typings is necessary to satisfy TypeScript, best to look here how it was solved:
kibana/src/plugins/discover/public/application/angular/doc_table/create_doc_table_react.tsx
Lines 128 to 140 in 3ad0532
useEffect(() => { | |
if (ref && ref.current && !scope.current) { | |
const fn = getRenderFn(ref.current, { ...renderProps, rows, minimumVisibleRows }); | |
fn().then((newScope) => { | |
scope.current = newScope; | |
}); | |
} else if (scope && scope.current) { | |
scope.current.renderProps = { ...renderProps, rows, minimumVisibleRows }; | |
scope.current.$apply(); | |
} | |
}, [renderProps, minimumVisibleRows, rows]); | |
useEffect(() => { |
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.
🎨 Optional chaining :)
ref?.current
scope?.current
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.
I think search_embeddable.ts
can be cleaned up?
src/plugins/discover/public/application/embeddable/saved_search_embeddable.tsx
Outdated
Show resolved
Hide resolved
be39589
to
2e9a9c8
Compare
@kertal I intentionally left |
@elasticmachine merge upstream |
const docTableProps = props as DocTableEmbeddableProps; | ||
return <DiscoverDocTableEmbeddable {...docTableProps} />; | ||
} | ||
const discoverGridProps = props as DiscoverGridProps; |
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.
This cast should be unnecessary.
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.
SearchProps
differ in more than just refs
from DiscoverGridProps
if (!this.searchProps) { | ||
return; | ||
} | ||
this.searchProps.refs = domNode; |
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.
I think we could improve readability and type saftyness if we'd not store the domNode
in the searchProps
. We have the domNode
available in the renderReactComponent
method, so we could explicitly pass it into the SaveSearchEmbeddableComponent
there besides the searchProps
, thus making us able to type the SavedSearchEmbeddableComponent
props in a way, that we know ref
is always there (instead of - as currently - type un-safe case the property type to something that has ref
explcitily non null on it.
tl;dr: Removing refs
from searchProps
, pass domNode
in renderReactComponent
explicitly to SavedSearchEmbeddableComponent
and fix types to match that.
export function SavedSearchEmbeddableComponent(props: SearchProps) { | ||
const { useLegacyTable } = props; | ||
if (useLegacyTable) { | ||
const docTableProps = props as DocTableEmbeddableProps; |
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.
See my comment above, on how we can get rid of this type un-safe cast.
|
||
import React from 'react'; | ||
|
||
import { DiscoverGridEmbeddable } from 'src/plugins/discover/public/application/angular/create_discover_grid_directive'; |
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.
Let's try to keep this import relative since it's within the same plugin, so we avoid later problems in case the overall plugin folder structure from the Kibana platform changes (once again :D)
if (scope.current) { | ||
scope.current.$destroy(); | ||
} |
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.
if (scope.current) { | |
scope.current.$destroy(); | |
} | |
scope.current?.$destroy(); |
const injector = await getServices().getEmbeddableInjector(); | ||
return await injectAngularElement(domNode, directive.template, props, injector); | ||
} catch (e) { | ||
render(<div>error</div>, domNode); |
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.
I assume we expect this to never happen. Since we still catch it, I think we should render a more meaningful message, that will help us trace this in case it would ever happen. If we expect it to "NEVER" happen, I think alternatively we might just not catch the error at all, and instead let it be handled by the overall embeddable error infrastructure.
} | ||
|
||
function getRenderFn(domNode: Element, props: DocTableEmbeddableProps) { | ||
const directive = { |
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.
🎨 Since this object has only one property, and doesn't seem to be used anywhere except for accessing that single property, maybe we could just make this a const template
instead.
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.
For consistency with other angular directives and overall readability, I'd like to keep it as is
} | ||
|
||
public reload() { | ||
if (this.searchProps) |
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.
🎨 I think we try not to skip the curly braces whenever possible, so I believe it might be a bit more aligned with the rest of the code if we'd also put that block into curly braces.
if (this.subscription) { | ||
this.subscription.unsubscribe(); | ||
} |
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.
🎨
if (this.subscription) { | |
this.subscription.unsubscribe(); | |
} | |
this.subscription?.unsubscribe(); |
return new Promise(() => { | ||
ReactDOM.render(<SavedSearchEmbeddableComponentMemoized {...searchProps} />, domNode); | ||
}); |
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.
❓ Could you clarify for me, why we create a new promise here? Are we trying to "delay" the rendering to the next execution slot for some reason? (Which would not work via that code, since the executor of a Promise is executed synchronously.) Because the way this is currently written we return a promise that will never resolve, and thus potentially creating a memory leak, since we're waiting on it in other places with await
(which will never pass).
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.
Matthias says it makes debugging easier :)
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.
but there's something missing in the code? one moment
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.
The 3rd param of ReactDom.render
will be executed once rendering has finished, it should resolve the Promise
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.
However fine with doing it simpler, main purpose was just to use 1 ReactDOM.render
instead of 2
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.
Ah okay, I see, you planned to do that as:
return new Promise((resolve) => {
ReactDOM.render(<SavedSearchEmbeddableComponentMemoized {...searchProps} />, domNode, resolve);
});
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.
If that actually helps lowering the amount of rendering, it might be a reasonable change. Though we should keep in mind, that the 3rd parameter or render
will be called on every Render or update, meaning React will also keep a reference around to that to call it again when updated, so I think we should make sure we're not accidentally leaking promises around between re-renderings (that anyway can only resolve once).
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.
used that to debug the issue about the dom node count increase, it wasn't the cause. fine with keeping it the way it was used before .. for now
src/plugins/discover/public/application/embeddable/saved_search_embeddable_component.tsx
Outdated
Show resolved
Hide resolved
src/plugins/discover/public/application/embeddable/saved_search_embeddable.tsx
Outdated
Show resolved
Hide resolved
@elasticmachine merge upstream |
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.
💚 Build SucceededMetrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
* [Discover] Render empty embeddable * First version of grid embeddable * More search embeddable * Almost stable version * Fixing typescript errors * Fixing filtering and sorting * Add data-shared-item to DiscoverGridEmbeddable * Trigger rerender when title changes * Fixing incorrectly touched files * Remove search_embeddable * Remove lodash * Fixing imports * Minor fixes * Removing unnecessary files * Minor fix * Remove unused import * Applying PR comments * Applying PR comments * Removing search embeddable * Fix missing import * Addressing PR comments * Do not memoize saved search component * Applying Matthias's suggestion Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> # Conflicts: # api_docs/deprecations.mdx
* master: [Security solution][Endpoint] Removes zip compression when creating artifacts (elastic#101379) [Dashboard]: Fixes disabled viz filter is applied (elastic#101859) [Discover] Deangularization of search embeddable (elastic#100552)
* [Discover] Render empty embeddable * First version of grid embeddable * More search embeddable * Almost stable version * Fixing typescript errors * Fixing filtering and sorting * Add data-shared-item to DiscoverGridEmbeddable * Trigger rerender when title changes * Fixing incorrectly touched files * Remove search_embeddable * Remove lodash * Fixing imports * Minor fixes * Removing unnecessary files * Minor fix * Remove unused import * Applying PR comments * Applying PR comments * Removing search embeddable * Fix missing import * Addressing PR comments * Do not memoize saved search component * Applying Matthias's suggestion Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> # Conflicts: # api_docs/deprecations.mdx Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
* [Discover] Render empty embeddable * First version of grid embeddable * More search embeddable * Almost stable version * Fixing typescript errors * Fixing filtering and sorting * Add data-shared-item to DiscoverGridEmbeddable * Trigger rerender when title changes * Fixing incorrectly touched files * Remove search_embeddable * Remove lodash * Fixing imports * Minor fixes * Removing unnecessary files * Minor fix * Remove unused import * Applying PR comments * Applying PR comments * Removing search embeddable * Fix missing import * Addressing PR comments * Do not memoize saved search component * Applying Matthias's suggestion Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Summary
This is an attempt to deangularize search embeddable. I've created a new component -
SavedSearchEmbeddable
that is instantiated by the factory. Much of the logic inside it remains the same, with a few caveats around lifecycle and updates.I've tested it for doc table extensively and a bit for DiscoverGrid.
Checklist
Delete any items that are not applicable to this PR.
- [ ] Documentation was added for features that require explanation or tutorials- [ ] Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)- [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)- [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list-~ [ ] This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)~
- [ ] This was checked for cross-browser compatibilityFor maintainers