-
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
[ML] Use context and custom hooks to manage legacy dependencies. #42244
Changes from 11 commits
5906cb9
c73c1fe
322c510
e04979f
00f8c23
a771ca5
76792ba
f168481
24a4483
5677cf1
87c7519
8c3cd59
99c7a9d
83953aa
f446c08
64b198a
41068f5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
export { KibanaContext, KibanaContextValue, SavedSearchQuery } from './kibana_context'; | ||
export { useKibanaContext } from './use_kibana_context'; | ||
export { useCurrentIndexPattern } from './use_current_index_pattern'; | ||
export { useCurrentSavedSearch } from './use_current_saved_search'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import React from 'react'; | ||
|
||
import { IndexPattern } from 'ui/index_patterns'; | ||
|
||
export interface KibanaContextValue { | ||
combinedQuery?: any; | ||
currentIndexPattern?: IndexPattern; | ||
currentSavedSearch?: any; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think this should be a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 64b198a. |
||
indexPatterns?: any; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think this should be an array of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 64b198a. I used the |
||
kbnBaseUrl?: string; | ||
kibanaConfig?: any; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think this should be a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 64b198a. ( |
||
} | ||
|
||
export type SavedSearchQuery = object; | ||
|
||
// This context provides dependencies which can be injected | ||
// via angularjs only (like services, currentIndexPattern etc.). | ||
// Because we cannot just import these dependencies, the default value | ||
// for the context is just {} and the value's type has optional | ||
// attributes for the angularjs based dependencies. Therefor, the | ||
// actual dependencies are set like we did previously with KibanaContext | ||
// in the wrapping angularjs directive. In the custom hook we check if | ||
// the dependencies are present with error reporting if they weren't | ||
// added properly. That's why in tests, these custom hooks must not | ||
// be mocked, instead <UiChrome.Provider value="mocked-value">` needs | ||
// to be used. This guarantees that we have both properly set up | ||
// TypeScript support and runtime checks for these dependencies. | ||
// Multiple custom hooks can be created to access subsets of | ||
// the overall context value if necessary too, | ||
// see useCurrentIndexPattern() for example. | ||
export const KibanaContext = React.createContext<KibanaContextValue>({}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { useContext } from 'react'; | ||
|
||
import { KibanaContext } from './kibana_context'; | ||
|
||
export const useCurrentIndexPattern = () => { | ||
const context = useContext(KibanaContext); | ||
|
||
if (context.currentIndexPattern === undefined) { | ||
throw new Error('currentIndexPattern is undefined'); | ||
} | ||
|
||
return context.currentIndexPattern; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { useContext } from 'react'; | ||
|
||
import { KibanaContext } from './kibana_context'; | ||
|
||
export const useCurrentSavedSearch = () => { | ||
const context = useContext(KibanaContext); | ||
|
||
if (context.currentSavedSearch === undefined) { | ||
throw new Error('currentSavedSearch is undefined'); | ||
} | ||
|
||
return context.currentSavedSearch; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { useContext } from 'react'; | ||
|
||
import { KibanaContext, KibanaContextValue } from './kibana_context'; | ||
|
||
export const useKibanaContext = () => { | ||
const context = useContext(KibanaContext); | ||
|
||
if ( | ||
context.combinedQuery === undefined || | ||
context.currentIndexPattern === undefined || | ||
context.currentSavedSearch === undefined || | ||
context.indexPatterns === undefined || | ||
context.kbnBaseUrl === undefined || | ||
context.kibanaConfig === undefined | ||
) { | ||
throw new Error('required attribute is undefined'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might be useful here to loop through the required attributes to check for undefined and then throw the error with the specific missing dependency. Just for future debugging purposes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this would potentially throw multiple times if more than one property is missing unless the loop builds up an array of missing properties and joins them for the error. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, the second thing you said. Should've been more clear. Not a must-have but would be nice to have. |
||
} | ||
|
||
return context as Required<KibanaContextValue>; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
export const uiChromeMock = { | ||
getBasePath: () => 'basePath', | ||
getUiSettingsClient: () => { | ||
return { | ||
get: (key: string) => { | ||
switch (key) { | ||
case 'dateFormat': | ||
case 'timepicker:timeDefaults': | ||
return {}; | ||
case 'timepicker:refreshIntervalDefaults': | ||
return { pause: false, value: 0 }; | ||
default: | ||
throw new Error(`Unexpected config key: ${key}`); | ||
} | ||
}, | ||
}; | ||
}, | ||
}; | ||
|
||
export const uiTimefilterMock = { | ||
getRefreshInterval: () => '30s', | ||
getTime: () => ({ from: 0, to: 0 }), | ||
on: (event: string, reload: () => void) => {}, | ||
}; | ||
|
||
export const uiTimeHistoryMock = { | ||
get: () => [{ from: 0, to: 0 }], | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { uiChromeMock } from './mocks'; | ||
|
||
export const useUiChromeContext = () => uiChromeMock; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { uiChromeMock, uiTimefilterMock, uiTimeHistoryMock } from './mocks'; | ||
|
||
export const useUiContext = () => ({ | ||
chrome: uiChromeMock, | ||
timefilter: uiTimefilterMock, | ||
timeHistory: uiTimeHistoryMock, | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
// We only export UiContext but not any custom hooks, because if we'd import them | ||
// from here, mocking the hook from jest tests won't work as expected. | ||
export { UiContext } from './ui_context'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import React from 'react'; | ||
|
||
import chrome from 'ui/chrome'; | ||
import { timefilter } from 'ui/timefilter'; | ||
import { timeHistory } from 'ui/timefilter/time_history'; | ||
|
||
// This provides ui/* based imports via React Context. | ||
// Because these dependencies can use regular imports, | ||
// they are just passed on as the default value | ||
// of the Context which means it's not necessary | ||
// to add <UiContext.Provider value="..." />... to the | ||
// wrapping angular directive, reducing a lot of boilerplate. | ||
// The custom hooks like useUiContext() need to be mocked in | ||
// tests because we rely on the properly set up default value. | ||
// Different custom hooks can be created to access parts only | ||
// from the full context value, see useUiChromeContext() as an example. | ||
export const UiContext = React.createContext({ | ||
chrome, | ||
timefilter, | ||
timeHistory, | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { useContext } from 'react'; | ||
|
||
import { UiContext } from './ui_context'; | ||
|
||
export const useUiChromeContext = () => { | ||
return useContext(UiContext).chrome; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { useContext } from 'react'; | ||
|
||
import { UiContext } from './ui_context'; | ||
|
||
export const useUiContext = () => { | ||
return useContext(UiContext); | ||
}; |
This file was deleted.
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'm unsure about this
combinedQuery
being in here. Firstly because I believe it's only used for creation pages, e.g. job wizards, data frame wizard.But mainly because it's not a standard kibana thing. it was originally created for the wizards to differentiate between the query you get from saved search and the result of combining that query with the filters and index pattern (also from the saved search) using kibana's
buildEsQuery
.Renaming it to
query
might be to general and cause confusion. I'm also not suggesting removing it now, as it's being used. but i think it's worth discussing as there may be other things like this which could go into their own context.Also I think its type should be
Query
from
import { Query } from 'src/legacy/core_plugins/data/public';
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.
It depends on
... = Private(SearchItemsProvider)
so it depends on being injected via angular (hence the previous change toAngularContext
which we reverted). Even if the name of the context isKibanaContext
now, its purpose is to pass on angularjs dependencies. I'm open to other suggestions but I'd like to address this in a follow up.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.
OK cool, I'm happy for it to stay in. We might need to revisit it later if most consumers of
KibanaContext
don't require it.