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

[Console] Fix leaky mappings subscription #45646

Merged
merged 3 commits into from
Sep 16, 2019

Conversation

jloleysens
Copy link
Contributor

Summary

Fixes the first issue mentioned here: #45032

Also fixes unmount never being called.

How to review

  1. Start Kibana + go to Console
  2. Check network tab (may be useful to set const POLL_INTERVAL = 60000; to a 10000 to see the requests at a higher rate).
  3. Navigate away from Console.
  4. Keep an eye on the network tab for any proxy?_alias... requests firing.

Release Note

Fixed a subscription in Console that didn't clear even after navigating away from the app.

@jloleysens jloleysens added Feature:Console Dev Tools Console Feature release_note:fix v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.5.0 labels Sep 13, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/es-ui

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@cjcenizal cjcenizal left a 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>;
Copy link
Contributor

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?

Copy link
Contributor Author

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!);
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@jloleysens jloleysens merged commit 8e18a5e into elastic:master Sep 16, 2019
jloleysens added a commit to jloleysens/kibana that referenced this pull request Sep 16, 2019
* Fix mappings subscription (still needs further refactoring)
Fix app not never unmounting

* More defensive removeChild behaviour on unmount
jloleysens added a commit to jloleysens/kibana that referenced this pull request Sep 16, 2019
…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
jloleysens added a commit that referenced this pull request Sep 16, 2019
* Fix mappings subscription (still needs further refactoring)
Fix app not never unmounting

* More defensive removeChild behaviour on unmount
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Console Dev Tools Console Feature release_note:fix Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.5.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants