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

Removing stateful saved object finder #52166

Merged
merged 14 commits into from
Dec 10, 2019

Conversation

flash1293
Copy link
Contributor

@flash1293 flash1293 commented Dec 4, 2019

This PR removes the legacy SavedObjectFinder and changes all usages to switch to the one provided by kibana_react. The biggest change is that it's now necessary to pass in uiSettings and savedObjects core services to the component.

@flash1293
Copy link
Contributor Author

Jenkins, test this.

@flash1293 flash1293 added release_note:skip Skip the PR/issue when compiling release notes v7.6.0 v8.0.0 labels Dec 4, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@flash1293 flash1293 marked this pull request as ready for review December 6, 2019 10:54
@flash1293 flash1293 requested a review from a team December 6, 2019 10:54
@flash1293 flash1293 requested review from a team as code owners December 6, 2019 10:54
@flash1293 flash1293 added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Dec 6, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)


interface Props {
onClose: () => void;
container: IContainer;
getFactory: GetEmbeddableFactory;
getAllFactories: GetEmbeddableFactories;
notifications: CoreSetup['notifications'];
SavedObjectFinder: React.ComponentType<any>;
savedObjects: CoreStart['savedObjects'];
uiSettings: CoreSetup['uiSettings'];
Copy link
Contributor

@majagrubic majagrubic Dec 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for my own understanding: what's the difference between CoreStart and CoreSetup? Everywhere else uiSettings is read from CoreStart, so why is it here defined as property of CoreSetup?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not cleanly done by me, I reverted those changes. The difference between CoreSetup and CoreStart is that they are services available during different startup phases of plugins. For the particular case of uiSettings, start and setup core should be identical so it wouldn't matter.

Copy link
Contributor Author

@flash1293 flash1293 Dec 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edit: Copied wrong link

@@ -174,7 +181,7 @@ class SavedObjectFinder extends React.Component<SavedObjectFinderProps, SavedObj
}
}, 300);

constructor(props: SavedObjectFinderProps) {
constructor(props: SavedObjectFinderUiProps) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was introducing this wrapper component necessary, instead of just introducing two new props on the existing SavedObjectFinder component? Are there still places where SavedObjectFinder is used, without uiSettings and savedObjects props?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not at the moment, but this is the target architecture we are moving towards (see also #52493 ). Passing uiSettings and savedObjects services down through the component tree can be cumbersome in some cases, so the wrapper component is able to pick it out of the Kibana context: https://github.com/elastic/kibana/tree/master/src/plugins/kibana_react#context

The usages of saved object finder in embeddables will move into this direction soon. Future uses should be guided into this direction.

Copy link
Contributor

@majagrubic majagrubic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, I just left two questions, mostly for my own understanding

Copy link
Contributor

@streamich streamich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you doing these changes to embeddables? We are in a process of refactoring embeddables where one does not need to pass all these services, like uiSettings and savedObject through props at every step but can instead consume them through React context. As I see it, your changes are basically step backwards that passes uiSettings and savedObjects services manually at every step through props—something we removed a couple of months ago.

Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM, tested in Discover, Visualize, Dashboard, Graph in Chrome, works as expected

@flash1293
Copy link
Contributor Author

After syncing with @streamich I reverted the changes in embeddables. This means consumers of embeddables are still required to pass in a SavedObjectFinder component. This will be changed once #52491 is implemented. Till then I moved the creation of the component into the places where it's still needed (similar to how it's done in https://github.com/elastic/kibana/blob/master/src/plugins/dashboard_embeddable_container/public/plugin.tsx#L64 )

Copy link
Contributor

@streamich streamich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

App Arch changes LGTM

Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ML and Transform changes LGTM

Copy link
Contributor

@crob611 crob611 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Canvas changes look good 👍

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@flash1293 flash1293 merged commit 3c57f71 into elastic:master Dec 10, 2019
flash1293 added a commit to flash1293/kibana that referenced this pull request Dec 10, 2019
jloleysens added a commit to jloleysens/kibana that referenced this pull request Dec 11, 2019
…le-sql-highlighting

* 'master' of github.com:elastic/kibana: (56 commits)
  Migrate url shortener service (elastic#50896)
  Re-enable datemath in from/to canvas timelion args (elastic#52159)
  [Logs + Metrics UI] Remove eslint exceptions (elastic#50979)
  [Logs + Metrics UI] Add missing headers in Logs & metrics (elastic#52405)
  [ML] API integration tests - initial tests for bucket span estimator (elastic#52636)
  [Watcher] New Platform (NP) Migration (elastic#50908)
  Decouple Authorization subsystem from Legacy API. (elastic#52638)
  [APM] Fix some warnings logged in APM tests (elastic#52487)
  [ui/public/utils] Delete unused base_object & find_by_param (elastic#52500)
  [ui/public/utils] Move items into ui/vis (elastic#52615)
  fix newlines in kbn-analytics build script
  Add top level examples folder and command to run, `--run-examples`. (elastic#52027)
  feat(NA): add trap for SIGINT in the git precommit hook (elastic#52662)
  [DOCS] Updtes description of elasticsearch.requestHeadersWhitelist (elastic#52675)
  [Telemetry/Pulse] Updates advanced settings text for usage data (elastic#52657)
  [SIEM][Detection Engine] Adds the default name space to the end of the signals index
  [Logs UI] Generalize ML module management (elastic#50662)
  Removing stateful saved object finder (elastic#52166)
  Shim oss telemetry (elastic#51168)
  [Reporting/Screenshots] Do not fail the report if request is aborted (elastic#52344)
  ...

# Conflicts:
#	src/legacy/core_plugins/console/public/legacy.ts
#	src/legacy/core_plugins/console/public/np_ready/application/models/legacy_core_editor/mode/elasticsearch_sql_highlight_rules.ts
#	src/legacy/core_plugins/console/public/np_ready/lib/autocomplete/components/full_request_component.ts
#	src/legacy/core_plugins/console/public/quarantined/src/sense_editor/row_parser.js
@spong spong mentioned this pull request Jan 29, 2020
26 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.6.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants