-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Console] Fix leaky mappings subscription #45646
Conversation
Fix app not never unmounting
Pinging @elastic/es-ui |
💚 Build Succeeded |
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 locally, code LGTM! Just had a couple questions. Can this be backported to 7.4 so it will ship with the next patch release of that minor? If so then we can add the 7.4.1
label to this PR.
@@ -64,12 +64,14 @@ uiRoutes.when('/dev_tools/console', { | |||
throw new Error(message); | |||
} | |||
|
|||
let unmount: AppUnmount | Promise<AppUnmount>; |
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 the scenario in which unmount would be assigned a promise value?
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 was done to satisfy the type constraints introduced by core: https://github.com/elastic/kibana/blob/master/src/core/public/application/types.ts#L82
@@ -99,7 +99,6 @@ export function useUIAceKeyboardMode(aceTextAreaElement: HTMLTextAreaElement | n | |||
if (aceTextAreaElement) { | |||
document.removeEventListener('keydown', documentKeyDownListener); | |||
aceTextAreaElement.removeEventListener('keydown', aceKeydownListener); | |||
document.removeChild(overlayMountNode.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.
Why is this being removed?
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 was running into the first of these two issues: https://developer.mozilla.org/en-US/docs/Web/API/Node/removeChild#Errors_thrown
Which indicated that cleanup for this DOM node is happening with the react component unmounting.
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.
Update: I re-implemented this in a more robust way.
@elasticmachine merge upstream |
💚 Build Succeeded |
* Fix mappings subscription (still needs further refactoring) Fix app not never unmounting * More defensive removeChild behaviour on unmount
…ole_app_data_flow * 'master' of github.com:elastic/kibana: [Console] Fix leaky mappings subscription (elastic#45646) TypeScriptify index_patterns/index_patterns/flatten_hit.js (elastic#45269) # Conflicts: # src/legacy/core_plugins/console/np_ready/public/application/containers/main/main.tsx # src/legacy/core_plugins/console/public/quarantined/src/app.js
Summary
Fixes the first issue mentioned here: #45032
Also fixes unmount never being called.
How to review
const POLL_INTERVAL = 60000;
to a10000
to see the requests at a higher rate).proxy?_alias...
requests firing.Release Note
Fixed a subscription in Console that didn't clear even after navigating away from the app.