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

Convert uiSettings service to TypeScript #47018

Merged
merged 18 commits into from Oct 4, 2019

Conversation

@restrry
Copy link
Member

commented Oct 1, 2019

Summary

Convert service to TS and declare IUiSettingsService public interface.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

restrry added 15 commits Sep 30, 2019
@elasticmachine

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2019

@restrry restrry requested a review from elastic/kibana-platform Oct 1, 2019
getAll: <T extends SavedObjectAttribute = any>() => Promise<Record<string, T>>;
getUserProvided: () => Promise<UserProvided>;
setMany: <T extends SavedObjectAttribute = any>(changes: Record<string, T>) => Promise<void>;
set: <T extends SavedObjectAttribute = any>(key: string, value: T) => Promise<void>;

This comment has been minimized.

Copy link
@restrry

restrry Oct 1, 2019

Author Member

value must satisfy SavedObjectAttribute` constraints. @rudolf is it okay that we cannot set an array of primitives? according to SavedObjects interface

This comment has been minimized.

Copy link
@rudolf

rudolf Oct 4, 2019

Contributor

I've created #47317 but see #47334 for some of the issues consumers of these types have experienced with the SavedObjectsClient which will be similar for the UiSettingsService.

@@ -233,7 +231,7 @@ describe('ui settings', () => {
});

try {
await uiSettings.setMany(['bar', 'foo']);
await uiSettings.setMany({ baz: 'baz', foo: 'foo' });

This comment has been minimized.

Copy link
@restrry

restrry Oct 1, 2019

Author Member

I believe it was used in unexpected way

// mock includeFrozen to return false
mockRequest.getUiSettingsService = () => ({ get: async () => false });
uiSettingsService.get.mockResolvedValue(false);
mockRequest.getUiSettingsService = () => uiSettingsService;

This comment has been minimized.

Copy link
@restrry

restrry Oct 1, 2019

Author Member

had to update tests to fix a type error

@@ -489,7 +487,7 @@ describe('ui settings', () => {
it('pulls user configuration from ES', async () => {
const esDocSource = {};
const { uiSettings, assertGetQuery } = setup({ esDocSource });
await uiSettings.get();
await uiSettings.get('any');

This comment has been minimized.

Copy link
@restrry

restrry Oct 1, 2019

Author Member

key is required argument

}

async getRaw() {
// NOTE: should be a private method
async getRaw(): Promise<UiSettingsRaw> {

This comment has been minimized.

Copy link
@restrry

restrry Oct 1, 2019

Author Member

right now we used them in tests. I'd re-write test and hide them later

get: <T extends SavedObjectAttribute = any>(key: string) => Promise<T>;
getAll: <T extends SavedObjectAttribute = any>() => Promise<Record<string, T>>;
getUserProvided: () => Promise<UserProvided>;
setMany: <T extends SavedObjectAttribute = any>(changes: Record<string, T>) => Promise<void>;

This comment has been minimized.

Copy link
@restrry

restrry Oct 1, 2019

Author Member

We can use T extends SavedObjectAttributes = any but not sure we should. I thought it made it harder to read the code.

@restrry restrry added the review label Oct 1, 2019
@restrry restrry added this to Code review in kibana-platform via automation Oct 1, 2019
await this._write({ changes });
}

async set(key, value) {
async set<T extends SavedObjectAttribute = any>(key: string, value: T) {

This comment has been minimized.

Copy link
@restrry

restrry Oct 1, 2019

Author Member

export interface IUiSettingsService {
getDefaults: () => Promise<Record<string, UiSettingsParams>>;
get: <T extends SavedObjectAttribute = any>(key: string) => Promise<T>;

This comment has been minimized.

Copy link
@restrry

restrry Oct 1, 2019

Author Member

used as

const value = uiSettings.get<string>('key');
// error
const value = uiSettings.get<string []>('key');
@sqren
sqren approved these changes Oct 1, 2019
Copy link
Member

left a comment

APM changes lgtm

* @property {Function} [options.log]
*/
constructor(options) {
export class UiSettingsService implements IUiSettingsService {

This comment has been minimized.

Copy link
@restrry

restrry Oct 1, 2019

Author Member

Shall we rename it to UiSettingsClient? It's not a service but a designated object for the plugins to interact with.

This comment has been minimized.

Copy link
@joshdover

joshdover Oct 2, 2019

Member

Yeah I think that makes sense, or even just UISettings?

This comment has been minimized.

Copy link
@joshdover

joshdover Oct 2, 2019

Member

Actually, we probably need to decide on our convention for capitalization of acronyms. For example we use HttpServiceSetup instead of HTTPServiceSetup in core right now. Should this be UiSettingsClient? I prefer this pattern because then the I-prefix looks a little less strange: IUiSettingsClient

@elasticmachine

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2019

@stacey-gammon stacey-gammon referenced this pull request Oct 1, 2019
3 of 10 tasks complete
@elasticmachine

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2019

@restrry

This comment has been minimized.

Copy link
Member Author

commented Oct 1, 2019

retest

@elasticmachine

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2019

export async function createOrUpgradeSavedConfig<T extends SavedObjectAttribute = any>(
options: Options
): Promise<Record<string, T> | undefined> {
const { savedObjectsClient, version, buildNum, logWithMetadata, onWriteError } = options;

This comment has been minimized.

Copy link
@joshdover

joshdover Oct 2, 2019

Member

Move object destructure to function arguments?

This comment has been minimized.

Copy link
@restrry

restrry Oct 4, 2019

Author Member

I'd prefer not to do. there are already too many params to use positional arguments.

@restrry

This comment has been minimized.

Copy link
Member Author

commented Oct 4, 2019

retest

@elasticmachine

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2019

@restrry restrry merged commit d1a99ea into elastic:master Oct 4, 2019
52 checks passed
52 checks passed
API integration tests node scripts/functional_tests --config test/api_integration/config.js --bail --debug
Details
Browser tests yarn run grunt test:browser-ci
Details
Build kbn_tp_sample_panel_action yarn build
Details
CLA All commits in pull request signed
Details
Check core API changes node scripts/check_core_api_changes
Details
Check file casing node scripts/check_file_casing --quiet
Details
Check licenses node scripts/check_licenses --dev
Details
Firefox smoke test node scripts/functional_tests --bail --debug --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/build/oss/kibana-8.0.0-SNAPSHOT-linux-x86_64 --include-tag smoke --config test/functional/config.firefox.js
Details
Functional tests / Group 1 yarn run grunt run:functionalTests_ciGroup1
Details
Functional tests / Group 10 yarn run grunt run:functionalTests_ciGroup10
Details
Functional tests / Group 11 yarn run grunt run:functionalTests_ciGroup11
Details
Functional tests / Group 12 yarn run grunt run:functionalTests_ciGroup12
Details
Functional tests / Group 2 yarn run grunt run:functionalTests_ciGroup2
Details
Functional tests / Group 3 yarn run grunt run:functionalTests_ciGroup3
Details
Functional tests / Group 4 yarn run grunt run:functionalTests_ciGroup4
Details
Functional tests / Group 5 yarn run grunt run:functionalTests_ciGroup5
Details
Functional tests / Group 6 yarn run grunt run:functionalTests_ciGroup6
Details
Functional tests / Group 7 yarn run grunt run:functionalTests_ciGroup7
Details
Functional tests / Group 8 yarn run grunt run:functionalTests_ciGroup8
Details
Functional tests / Group 9 yarn run grunt run:functionalTests_ciGroup9
Details
Internationalization check node scripts/i18n_check --ignore-missing
Details
Interpreter functional tests node scripts/functional_tests --config test/interpreter_functional/config.js --bail --debug --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/build/oss/kibana-8.0.0-SNAPSHOT-linux-x86_64-1
Details
Jest integration tests yarn run grunt test:jest_integration
Details
Jest tests yarn run grunt test:jest
Details
Mocha tests node scripts/mocha
Details
Plugin functional tests node scripts/functional_tests --config test/plugin_functional/config.js --bail --debug --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/build/oss/kibana-8.0.0-SNAPSHOT-linux-x86_64-1
Details
Project tests yarn run grunt test:projects
Details
Type check node scripts/type_check
Details
TypeScript - all files belong to a TypeScript project node scripts/check_ts_projects
Details
Verify NOTICE.txt node scripts/notice --validate
Details
Verify dependency versions yarn run grunt verifyDependencyVersions
Details
X-Pack Chrome Functional tests / Group 1 node scripts/functional_tests --debug --bail --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/install/kibana-1 --include-tag ciGroup1
Details
X-Pack Chrome Functional tests / Group 10 node scripts/functional_tests --debug --bail --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/install/kibana-10 --include-tag ciGroup10
Details
X-Pack Chrome Functional tests / Group 2 node scripts/functional_tests --debug --bail --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/install/kibana-2 --include-tag ciGroup2
Details
X-Pack Chrome Functional tests / Group 3 node scripts/functional_tests --debug --bail --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/install/kibana-3 --include-tag ciGroup3
Details
X-Pack Chrome Functional tests / Group 4 node scripts/functional_tests --debug --bail --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/install/kibana-4 --include-tag ciGroup4
Details
X-Pack Chrome Functional tests / Group 5 node scripts/functional_tests --debug --bail --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/install/kibana-5 --include-tag ciGroup5
Details
X-Pack Chrome Functional tests / Group 6 node scripts/functional_tests --debug --bail --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/install/kibana-6 --include-tag ciGroup6
Details
X-Pack Chrome Functional tests / Group 7 node scripts/functional_tests --debug --bail --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/install/kibana-7 --include-tag ciGroup7
Details
X-Pack Chrome Functional tests / Group 8 node scripts/functional_tests --debug --bail --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/install/kibana-8 --include-tag ciGroup8
Details
X-Pack Chrome Functional tests / Group 9 node scripts/functional_tests --debug --bail --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/install/kibana-9 --include-tag ciGroup9
Details
X-Pack Jest node scripts/jest --ci --verbose
Details
X-Pack Mocha yarn test
Details
X-Pack SIEM cyclic dependency test node legacy/plugins/siem/scripts/check_circular_deps
Details
X-Pack firefox smoke test node scripts/functional_tests --debug --bail --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/install/kibana-11 --include-tag smoke --config test/functional/config.firefox.js
Details
elasticsearch-ci/docs Build finished.
Details
eslint node scripts/eslint --no-cache
Details
kibana-ci Build finished.
Details
prbot:outdated
prbot:release note labels
prbot:release version labels
sasslint node scripts/sasslint
Details
kibana-platform automation moved this from Code review to Needs Backport Oct 4, 2019
@restrry restrry deleted the restrry:ts-ui-settings-code branch Oct 4, 2019
restrry added a commit to restrry/kibana that referenced this pull request Oct 4, 2019
* tsify is_config_version_upgradeable

* tsify get_upgradeable_config

* tsify create_or_upgrade_saved_config

* tsify ui_settings_service

* tsify ui_settings_service_factory

* tsify ui_settings_service_for_request

* declare types on server object

* tsify set route

* tsify set_many route

* tsify get route

* tsify delete route

* tsify logWithMetadata

* improve ui_settings_service typings

* introduce uiService mocks

* remove private methods from public contract

* add types for server.uiSettingsServiceFactory

* rename IUiSettingsService --> IUiSettingsClient
restrry added a commit that referenced this pull request Oct 4, 2019
* tsify is_config_version_upgradeable

* tsify get_upgradeable_config

* tsify create_or_upgrade_saved_config

* tsify ui_settings_service

* tsify ui_settings_service_factory

* tsify ui_settings_service_for_request

* declare types on server object

* tsify set route

* tsify set_many route

* tsify get route

* tsify delete route

* tsify logWithMetadata

* improve ui_settings_service typings

* introduce uiService mocks

* remove private methods from public contract

* add types for server.uiSettingsServiceFactory

* rename IUiSettingsService --> IUiSettingsClient
@rudolf rudolf referenced this pull request Oct 4, 2019
0 of 7 tasks complete
@joshdover joshdover moved this from Needs Backport to Done (7.5) in kibana-platform Oct 4, 2019
@restrry restrry added backported and removed review labels Oct 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
kibana-platform
  
Done (7.5)
5 participants
You can’t perform that action at this time.