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

[Data] Query Input String manager #72093

Merged
merged 47 commits into from
Jul 29, 2020
Merged

Conversation

lizozom
Copy link
Contributor

@lizozom lizozom commented Jul 16, 2020

Summary

  • Allow gracefully extracting the query string state, to be consumed by other services. One can now use the data.query.state$ observable and receive all state updates in one place.
  • Add the data.query.queryString service, allowing to set the query string programmatically.
  • Use the data.query.queryString.getDefaultQuery() anywhere that previously constructed the default query from scratch.

Dev Docs

This PR allows gracefully extracting the query string state, to be consumed by other services.
One can now use the data.query.state$ observable and receive all state updates in one place.

data.query.state$.subscribe((queryState: QueryState) => {...})

This PR also adds the data.query.queryString service, allowing to set the query string programmatically.

data.query.queryString.setQuery({query: 'abc', language: 'kuery'});

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@lizozom lizozom self-assigned this Jul 16, 2020
@lizozom lizozom requested a review from Dosant July 16, 2020 16:06
@lizozom lizozom mentioned this pull request Jul 16, 2020
7 tasks
@Dosant
Copy link
Contributor

Dosant commented Jul 20, 2020

ACK will play with it

@lizozom lizozom marked this pull request as ready for review July 22, 2020 07:14
@lizozom lizozom requested a review from a team July 22, 2020 07:14
@lizozom lizozom requested a review from a team as a code owner July 22, 2020 07:14
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

@flash1293
Copy link
Contributor

@lizozom Can you expand a little on the context of this PR and why things are changed?

@lizozom
Copy link
Contributor Author

lizozom commented Jul 23, 2020

@elasticmachine merge upstream

@@ -292,6 +291,8 @@ export class MapsAppView extends React.Component {
globalState,
}),
};

if (newState.query) queryString.setQuery(newState.query);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nreese I had to add this line, setting the query to the manager in order to stabilize functional tests.
Is this addition correct and in the proper location in your opinion?

Copy link
Contributor

Choose a reason for hiding this comment

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

This component needs some refactoring. This is a good place to put this until that refactoring occurs.

@lizozom lizozom requested a review from Dosant July 27, 2020 13:29
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

async chunks size

id value diff baseline
discover 430.9KB -280.0B 431.2KB
indexPatternManagement 697.5KB -45.0B 697.6KB
lens 24.7KB -204.0B 24.9KB
maps 3.8MB -150.0B 3.8MB
visualize 676.9KB -372.0B 677.3KB
total - -1.0KB -

page load bundle size

id value diff baseline
charts 873.6KB -40.0B 873.7KB
dashboard 679.3KB -334.0B 679.6KB
data 1.4MB +3.2KB 1.4MB
regionMap 832.2KB -40.0B 832.3KB
tileMap 843.8KB -40.0B 843.9KB
visTypeMarkdown 552.8KB -40.0B 552.8KB
visTypeMetric 575.8KB -40.0B 575.9KB
visTypeTable 604.6KB -40.0B 604.6KB
visTypeTagcloud 837.1KB -40.0B 837.1KB
visTypeTimelion 712.5KB -40.0B 712.6KB
visTypeVega 660.6KB -40.0B 660.6KB
visTypeVislib 1.3MB -40.0B 1.3MB
total - +2.5KB -

History

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

Copy link
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

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

I tested dashboard, discover and state management demo plugin in chrome and safari and didn't find regressions.

Probably also worth requesting re-review from @elastic/kibana-app, since there were significant changes


export const useQueryStringManager = (props: UseQueryStringProps) => {
// Filters should be either what's passed in the initial state or the current state of the filter manager
const [query, setQuery] = useState<Query>(props.query || props.queryStringManager.getQuery());
Copy link
Contributor

Choose a reason for hiding this comment

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

As I understood props.query are only used on initial rendering for initial state? If this is the case, let's rename it into props.initialQuery for clarity?

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'd want any change to all attributes (query, filters, time, etc)
So lets take this separately :)

Copy link
Contributor

@ThomThomson ThomThomson left a 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, lots of changes, but no regressions I could find. Also looked into the new data.query.state$, and programatically setting the query string which both work great!

@lizozom lizozom merged commit 1414415 into elastic:master Jul 29, 2020
lizozom added a commit to lizozom/kibana that referenced this pull request Jul 29, 2020
* improve test stability

* query string input manager (needed for search demo)

* docs

* dashboard

* Fix jest

* mock fix

* Allow restoring a saved query

* sync url

* Luke's fix to test

* cleanup

* lens jest tests

* docs

* use queryStringManager.getDefaultQuery
Don't sync query to global state

* Update app.test.tsx

lens mock

* jest fix

* jest

* use new api in the example

* Rename state param to query to match url state

* Apply changes to discover

* Update src/plugins/data/public/query/query_string/index.ts

Co-authored-by: Anton Dosov <[email protected]>

* Improve query string state manager

* Cleanup dashboard code

* Handle refresh button

* Set initial dashboard state

* visualize state

* remove unused

* docs

* fix example

* fix jest

* fix filter app state in discover

* fix maps test

* jest

Co-authored-by: Anton Dosov <[email protected]>
Co-authored-by: Anton Dosov <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
# Conflicts:
#	src/plugins/data/public/public.api.md
lizozom added a commit that referenced this pull request Jul 29, 2020
* [Data] Query Input String manager (#72093)

* improve test stability

* query string input manager (needed for search demo)

* docs

* dashboard

* Fix jest

* mock fix

* Allow restoring a saved query

* sync url

* Luke's fix to test

* cleanup

* lens jest tests

* docs

* use queryStringManager.getDefaultQuery
Don't sync query to global state

* Update app.test.tsx

lens mock

* jest fix

* jest

* use new api in the example

* Rename state param to query to match url state

* Apply changes to discover

* Update src/plugins/data/public/query/query_string/index.ts

Co-authored-by: Anton Dosov <[email protected]>

* Improve query string state manager

* Cleanup dashboard code

* Handle refresh button

* Set initial dashboard state

* visualize state

* remove unused

* docs

* fix example

* fix jest

* fix filter app state in discover

* fix maps test

* jest

Co-authored-by: Anton Dosov <[email protected]>
Co-authored-by: Anton Dosov <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
# Conflicts:
#	src/plugins/data/public/public.api.md

* docs
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 29, 2020
* master: (126 commits)
  [ML] Disabling ML if license feature is disabled (elastic#73187)
  [ML] Fixing old _xpack style es endpoint paths (elastic#73667)
  [DOCS] [Lens] 7.9 docs refresh (elastic#72301)
  [ML] DF Analytics results: ensure `View` link is only enabled when job has successfully completed (elastic#73539)
  Set timeRange to default to trigger the error message (elastic#73629)
  [ML] Functional tests - stabilize DFA navigation and index pattern handling (elastic#73660)
  [ILM] Add links to "Snapshot and Restore" from ILM "wait for snapshot policy" (elastic#72473)
  [kbn-storybook] Update Storybook to 5.3.19 (elastic#73320)
  [Metrics UI] Fix hasData call to ensure it has data not just indices (elastic#72969)
  [Uptime] Use `service.name` to link from Uptime -> APM where available (elastic#73618)
  allow others to update `URL.revokeObjectURL` property if needed (elastic#73639)
  regen docs (elastic#73650)
  [Visualize] Fix inspector download filename issue when saving in-place (elastic#72605)
  [Data] Query Input String manager (elastic#72093)
  [Security Solutions] Add tooltips (elastic#73436)
  Do not render descriptionless actions within an EuiCard (elastic#73611)
  [Security Solution][Detections] Value Lists Modal supports multiple exports (elastic#73532)
  [Security Solution][Resolver] Handle disabled process collection (elastic#73592)
  [Security_Solution][Bug] Fix user name/domain to ECS structure (elastic#73530)
  [Security Solution][Exceptions] - Update rule.exceptions_list to include exception list list_id (elastic#73349)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 30, 2020
…ibana into actions/webhook-remove-header

* 'actions/webhook-remove-header' of github.com:gmmorris/kibana: (86 commits)
  [maps] rename GisMap to MapContainer and convert to TS (elastic#73690)
  [APM] docs: remove watcher documentation  (elastic#73485)
  [Maps] fix fit to data for Point to Point layer (elastic#73563)
  [Metrics UI] Fix No Data in Inventory alerts/Snapshot API (elastic#72513)
  [ML] Disabling ML if license feature is disabled (elastic#73187)
  [ML] Fixing old _xpack style es endpoint paths (elastic#73667)
  [DOCS] [Lens] 7.9 docs refresh (elastic#72301)
  [ML] DF Analytics results: ensure `View` link is only enabled when job has successfully completed (elastic#73539)
  Set timeRange to default to trigger the error message (elastic#73629)
  [ML] Functional tests - stabilize DFA navigation and index pattern handling (elastic#73660)
  [ILM] Add links to "Snapshot and Restore" from ILM "wait for snapshot policy" (elastic#72473)
  [kbn-storybook] Update Storybook to 5.3.19 (elastic#73320)
  [Metrics UI] Fix hasData call to ensure it has data not just indices (elastic#72969)
  [Uptime] Use `service.name` to link from Uptime -> APM where available (elastic#73618)
  allow others to update `URL.revokeObjectURL` property if needed (elastic#73639)
  regen docs (elastic#73650)
  [Visualize] Fix inspector download filename issue when saving in-place (elastic#72605)
  [Data] Query Input String manager (elastic#72093)
  [Security Solutions] Add tooltips (elastic#73436)
  Do not render descriptionless actions within an EuiCard (elastic#73611)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Query Bar Querying and query bar features release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants