Migrate base path APIs and UiSettings client to new platform #22694

spalger · 2018-09-04T21:28:57Z

Fixes #20697

This PR migrates the base path related methods from ui/chrome to core.basePath and the uiSettings to core.uiSettings. The two are not split into separate PRs because I'm kinda cramped for time right now so rather I split the changes up into two commits so that you can review them separately if you like. If you'd like I can submit them as separate PRs but the basePath PR will block the uiSettings PR either way.

There shouldn't be any API changes except one thing, since the existing implementation is so close to what we'd want from the new platform API I made the one change that is inconsistent with what we've been doing so far and moved uiSettings.subscribe() to uiSettings.getUpdate$().subscribe(). This method isn't super commonly used, but it is a breaking change that will likely impact plugins so I'll notify some folks if we decide to move forward this way. I can also make a super-light wrapper for angular that just updates this method if you prefer.

epixa · 2018-09-04T21:49:55Z

I did a high level review of the code in the base path commit, and it looks pretty good. The modifyUrl code seemed to be the only thing that wasn't completely straightforward, but I figured out that was actually pulled from the legacy world, so it works for me. The code itself wasn't bad or anything, but if it was all net new, I'd want to question some of the decisions with it (e.g. "why standardize on this property instead of that one?"), but that seems beyond the scope of this PR. In the end, that code is working well in production and everyone is fine with the API mechanics, which this PR doesn't change.

elasticmachine · 2018-09-05T00:41:03Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

spalger · 2018-09-05T19:49:39Z

src/core/public/ui_settings/ui_settings_service.test.ts

+ * under the License.
+ */
+
+function mockClass<T>(


@azasypkin Tried making a helper for mocking classes again, and this time I really like how it turned out. Thoughts?

Paid a little extra attention to make it serialize in snapshots nicely too: https://github.com/elastic/kibana/pull/22694/files#diff-de8d63302459e4f07e8b9f006649574fR17

I like it! We can probably move it to some shared test tools/utils and use it in other tests from now on. Just two things:

I can't think of any real use case right now, but it may happen that file/module exports both class and something else that we'd like to mock and we won't be able to use mockClass for that or I'm wrong?

Wondering if we can use jest-mock's generateMetadata and generateFromMetadata to mock all class methods at once and then set behavior/return values in setup function.

Cool, I'll follow up with something like that if/when I need this util again and will investigate how we can support modules with more exports. I've tried using the autoMocks before but found them to be really mysterious and the docs didn't really help me understand what was going on... not opposed to them, but since I've struggled to figure them out til this point I worry they might make the tests harder to understand... but really just don't know.

…ation did

elasticmachine · 2018-09-05T22:08:37Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

azasypkin · 2018-09-06T13:49:27Z

ack: started to look into PR already, will try to finish today, tomorrow is more realistic though.

legrego · 2018-09-06T18:23:02Z

@spalger -- I didn't see anything jump out at me in this PR, but the title scared me, so I'm here to ask if this will have any impact on the base path work that Spaces is built on? Specifically, we moved the basePath determination from just using server.basePath to a combination of server.basePath, and the space identifier, which is determined at the request level.

Our implementation is essentially stolen from your previous work on Spaces (thanks!): https://github.com/elastic/kibana/pull/18664/files

spalger · 2018-09-06T19:29:21Z

@legrego This won't have any impact on the server-side handling of base path, it just exposes the basePath information that is currently being injected into the UI via the new platform which is then injected into ui/chrome/apis/base_path to power the existing APIs. It's mostly just shifting around how we expose the basePath client-side without changing anything about the way it's derived. As long as the spaces changes update this then everything should work nicely

…latform/ui-settings-client

…ed ones

elasticmachine · 2018-09-06T22:09:43Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…#22694 and break with jest 23

azasypkin

Looks great, I haven't noticed anything suspicious while testing locally. Left a bunch of questions and minor nits, but nothing that would block this PR.

azasypkin · 2018-09-07T09:56:51Z

src/core/public/injected_metadata/injected_metadata_service.ts

+      },
+
+      getKibanaVersion: () => {
+        return this.getKibanaVersion();


note: haha, my IDE mistakenly marks this as a recursive call :)

src/core/public/base_path/base_path_service.ts

src/core/public/utils/modify_url.ts

azasypkin · 2018-09-07T14:38:05Z

src/core/public/ui_settings/ui_settings_client.ts

+  }
+
+  /**
+   * Prepares the uiSettingsClient to be discarded, completing any update$ observerables


typo: observables

azasypkin · 2018-09-07T14:50:09Z

src/core/public/ui_settings/ui_settings_client.ts

+    const declared = this.isDeclared(key);
+    const defaults = this.defaults;
+
+    const oldVal = declared ? this.cache[key].userValue : undefined;


question: do you have any clue why we needed this before?

const newVal = key in defaults && defaults[key].defaultValue === value ? null : value;

We didn't, I checked the code since it was first introduced and best I can tell defaults[key].defaultValue was never actually defined, so I just got rid of it.

azasypkin · 2018-09-07T15:45:42Z

src/core/public/ui_settings/ui_settings_service.test.ts

+    expect(start).toBeInstanceOf(MockUiSettingsClient);
+  });
+
+  it('contstructs UiSettingsClient and UiSettingsApi', () => {


typo: constructs.

azasypkin · 2018-09-07T16:09:19Z

src/core/public/ui_settings/ui_settings_service.test.ts

+ * under the License.
+ */
+
+function mockClass<T>(


I like it! We can probably move it to some shared test tools/utils and use it in other tests from now on. Just two things:

I can't think of any real use case right now, but it may happen that file/module exports both class and something else that we'd like to mock and we won't be able to use mockClass for that or I'm wrong?

Wondering if we can use jest-mock's generateMetadata and generateFromMetadata to mock all class methods at once and then set behavior/return values in setup function.

azasypkin · 2018-09-07T16:10:26Z

src/core/public/ui_settings/types.ts

+ */
+
+// properties that come from legacyInjectedMetadata.uiSettings.defaults
+interface InjectedUiSettingsDefault {


question: where did you get these "schemas" from?

They're really just snapshots of what we get based on what's in the uiExports today. They can absolutely go out of date because they really aren't validated or enforced anywhere, which I plan to fix in #22779

…latform/ui-settings-client

elasticmachine · 2018-09-08T01:30:40Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

I'd really like to upgrade to Typescript 3 for its `unknown` type, but we need to upgrade to `jest@23` to support a recent version of `ts-jest@23`. The [jest changelog](https://github.com/facebook/jest/blob/master/CHANGELOG.md) breaks down the breaking changes in 23.x, but I found it to be slightly incomplete so I've broken down the changes that actually caused breaks for us here, and addressed each in individual commits to make review a little easier: - the `testURL` config default was changed from `about:blank` to `http://localhost` - this cause some XHR requests powered by JSdom to start failing. It seems these requests just do nothing in master but start to fail when JSdom is initialized with an actual URL... I think we would ideally stop sending meaningless XHR requests in the tests, but it was a lot easier to just set the config to `about:blank` for now, and we can worry about cleanup later if necessary - `expect(...).toThrow()` only passes if an actual error was thrown. - In two places in the index pattern code we were throwing strings, which broke the assertions. Fortunately/Unfortunately the errors are not being consumed by anything, so I was able to wrap them in `new Error()` without causing any issues. - snapshots of mock functions now include a `results` array, detailing the return values of the function - React fragments are now serialized as `<React.Fragment>` instead of `<UNDEFINED>` - undefined props in React components are now stripped from snapshots - minor changes to the ordering of mocks, imports resolution, and before hooks caused the uiSettings API tests to start breaking, but I'm replacing them with totally new tests in #22694 so I just deleted them here - mocks created with `jest.spyOn()` that are restored now have their `mock.calls` reset, so some of the kbn-pm tests stated failing. This was fixed by restoring them with `jest.restoreAllMocks()` rather than trying to do it before the assertions

I'd really like to upgrade to Typescript 3 for its `unknown` type, but we need to upgrade to `jest@23` to support a recent version of `ts-jest@23`. The [jest changelog](https://github.com/facebook/jest/blob/master/CHANGELOG.md) breaks down the breaking changes in 23.x, but I found it to be slightly incomplete so I've broken down the changes that actually caused breaks for us here, and addressed each in individual commits to make review a little easier: - the `testURL` config default was changed from `about:blank` to `http://localhost` - this cause some XHR requests powered by JSdom to start failing. It seems these requests just do nothing in master but start to fail when JSdom is initialized with an actual URL... I think we would ideally stop sending meaningless XHR requests in the tests, but it was a lot easier to just set the config to `about:blank` for now, and we can worry about cleanup later if necessary - `expect(...).toThrow()` only passes if an actual error was thrown. - In two places in the index pattern code we were throwing strings, which broke the assertions. Fortunately/Unfortunately the errors are not being consumed by anything, so I was able to wrap them in `new Error()` without causing any issues. - snapshots of mock functions now include a `results` array, detailing the return values of the function - React fragments are now serialized as `<React.Fragment>` instead of `<UNDEFINED>` - undefined props in React components are now stripped from snapshots - minor changes to the ordering of mocks, imports resolution, and before hooks caused the uiSettings API tests to start breaking, but I'm replacing them with totally new tests in elastic#22694 so I just deleted them here - mocks created with `jest.spyOn()` that are restored now have their `mock.calls` reset, so some of the kbn-pm tests stated failing. This was fixed by restoring them with `jest.restoreAllMocks()` rather than trying to do it before the assertions

…latform/ui-settings-client

… angular

I'd really like to upgrade to Typescript 3 for its `unknown` type, but we need to upgrade to `jest@23` to support a recent version of `ts-jest@23`. The [jest changelog](https://github.com/facebook/jest/blob/master/CHANGELOG.md) breaks down the breaking changes in 23.x, but I found it to be slightly incomplete so I've broken down the changes that actually caused breaks for us here, and addressed each in individual commits to make review a little easier: - the `testURL` config default was changed from `about:blank` to `http://localhost` - this cause some XHR requests powered by JSdom to start failing. It seems these requests just do nothing in master but start to fail when JSdom is initialized with an actual URL... I think we would ideally stop sending meaningless XHR requests in the tests, but it was a lot easier to just set the config to `about:blank` for now, and we can worry about cleanup later if necessary - `expect(...).toThrow()` only passes if an actual error was thrown. - In two places in the index pattern code we were throwing strings, which broke the assertions. Fortunately/Unfortunately the errors are not being consumed by anything, so I was able to wrap them in `new Error()` without causing any issues. - snapshots of mock functions now include a `results` array, detailing the return values of the function - React fragments are now serialized as `<React.Fragment>` instead of `<UNDEFINED>` - undefined props in React components are now stripped from snapshots - minor changes to the ordering of mocks, imports resolution, and before hooks caused the uiSettings API tests to start breaking, but I'm replacing them with totally new tests in #22694 so I just deleted them here - mocks created with `jest.spyOn()` that are restored now have their `mock.calls` reset, so some of the kbn-pm tests stated failing. This was fixed by restoring them with `jest.restoreAllMocks()` rather than trying to do it before the assertions

elasticmachine · 2018-09-08T04:06:03Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…#22694) Fixes elastic#20697 This PR migrates the base path related methods from `ui/chrome` to `core.basePath` and the uiSettings to `core.uiSettings`. The two are not split into separate PRs because I'm kinda cramped for time right now so rather I split the changes up into two commits so that you can review them separately if you like. If you'd like I can submit them as separate PRs but the basePath PR will block the uiSettings PR either way. There shouldn't be any API changes except one thing, since the existing implementation is so close to what we'd want from the new platform API I made the one change that is inconsistent with what we've been doing so far and moved `uiSettings.subscribe()` to `uiSettings.getUpdate$().subscribe()`. This method isn't super commonly used, but it is a breaking change that will likely impact plugins so I'll notify some folks if we decide to move forward this way. I can also make a super-light wrapper for angular that just updates this method if you prefer.

…22694) (#22848) Backports the following commits to 6.x: - Migrate base path APIs and UiSettings client to new platform (#22694)

spalger · 2018-09-08T10:42:28Z

6.x/6.5: 0161da8

spalger added 2 commits September 4, 2018 13:08

create basePath service for new platform

d02cd0e

migrate ui settings to new platform

68f7f01

spalger added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform v7.0.0 v6.5.0 labels Sep 4, 2018

[ui/core/uiSetttings] actually await expect(promise) calls

743b553

spalger added 2 commits September 5, 2018 12:34

Merge branch 'master' of github.com:elastic/kibana into HEAD

9618563

[ui/core/uiSettings] add tests for uiSettingsService

df345e1

spalger force-pushed the migrate-new-platform/ui-settings-client branch from 23ed10e to df345e1 Compare September 5, 2018 19:36

spalger commented Sep 5, 2018

View reviewed changes

test out mock class helper

05d9b8e

spalger force-pushed the migrate-new-platform/ui-settings-client branch from b5eb2eb to 05d9b8e Compare September 5, 2018 20:01

spalger added 2 commits September 5, 2018 13:21

fix reference to uiSettings.subscribe()

b772a7d

[ui/chrome/test] derive basePath from internals like legacy implement…

4691b7b

…ation did

spalger requested a review from azasypkin September 5, 2018 22:20

spalger added 2 commits September 6, 2018 12:35

Merge branch 'master' of github.com:elastic/kibana into migrate-new-p…

dc9f43b

…latform/ui-settings-client

[core/public/uiSettingsClient/types] include all properties, even uns…

dc5ddc7

…ed ones

This comment has been minimized.

Sign in to view

spalger mentioned this pull request Sep 6, 2018

Migrate injected UiSettings from legacyMetadata #22779

Open

spalger pushed a commit to spalger/kibana that referenced this pull request Sep 7, 2018

[uiSettings] remove uiSettings tests that will be replaced in elastic…

15e2483

…#22694 and break with jest 23

azasypkin approved these changes Sep 7, 2018

View reviewed changes

spalger added 12 commits September 7, 2018 17:17

fix typos

02f0cc9

limit exported types to those in use

63bcb4d

remove unnecessary copy of modify_url util in ui/public

f5e6643

remove unnecessary types from JSDoc

1bcd411

use Number constructor rather than parseInt

60d8924

[core/public/legacyService] test that uiSettings is initialized

cf2e10f

[core/public/uiSettingsClient] use readonly when possible

a7db8fd

[core/public/uiSettingsClient] use imported type since available

ea4cf06

[core/public/uiSettingsClient] remove unnecessary Boolean cast

ddbd2b8

Merge branch 'master' of github.com:elastic/kibana into migrate-new-p…

8dec538

…latform/ui-settings-client

[core/public/basePath/removeBasePath] handle a couple edge-cases better

d2f5816

fix modifyUrl import

aa60f3e

fix another modifyUrl import (this time run eslint to verify :P)

aac0a7c

spalger and others added 3 commits September 7, 2018 18:45

Merge branch 'master' of github.com:elastic/kibana into migrate-new-p…

19eb622

…latform/ui-settings-client

[jest] update snapshots

b1a6ddf

import directly from core when necessary to avoid infecting jest with…

596ca89

… angular

spalger merged commit 52060b8 into elastic:master Sep 8, 2018

spalger mentioned this pull request Sep 8, 2018

[6.x] Migrate base path APIs and UiSettings client to new platform (#22694) #22848

Merged

spalger pushed a commit that referenced this pull request Sep 8, 2018

[6.x] Migrate base path APIs and UiSettings client to new platform (#…

0161da8

…22694) (#22848) Backports the following commits to 6.x: - Migrate base path APIs and UiSettings client to new platform (#22694)

spalger deleted the migrate-new-platform/ui-settings-client branch September 8, 2018 10:42

legrego mentioned this pull request Sep 12, 2018

[Spaces] - Space-aware Settings UI #22886

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate base path APIs and UiSettings client to new platform #22694

Migrate base path APIs and UiSettings client to new platform #22694

spalger commented Sep 4, 2018 •

edited

Loading

epixa commented Sep 4, 2018

elasticmachine commented Sep 5, 2018

spalger Sep 5, 2018 •

edited

Loading

spalger Sep 5, 2018

azasypkin Sep 7, 2018

spalger Sep 8, 2018

elasticmachine commented Sep 5, 2018

azasypkin commented Sep 6, 2018

legrego commented Sep 6, 2018

spalger commented Sep 6, 2018

This comment has been minimized.

elasticmachine commented Sep 6, 2018

azasypkin left a comment

azasypkin Sep 7, 2018

azasypkin Sep 7, 2018

azasypkin Sep 7, 2018

spalger Sep 8, 2018

azasypkin Sep 7, 2018

azasypkin Sep 7, 2018

azasypkin Sep 7, 2018

spalger Sep 8, 2018

elasticmachine commented Sep 8, 2018

elasticmachine commented Sep 8, 2018

spalger commented Sep 8, 2018

Migrate base path APIs and UiSettings client to new platform #22694

Migrate base path APIs and UiSettings client to new platform #22694

Conversation

spalger commented Sep 4, 2018 • edited Loading

epixa commented Sep 4, 2018

elasticmachine commented Sep 5, 2018

💔 Build Failed

spalger Sep 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Sep 5, 2018

💚 Build Succeeded

azasypkin commented Sep 6, 2018

legrego commented Sep 6, 2018

spalger commented Sep 6, 2018

This comment has been minimized.

elasticmachine commented Sep 6, 2018

💚 Build Succeeded

azasypkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Sep 8, 2018

💔 Build Failed

elasticmachine commented Sep 8, 2018

💚 Build Succeeded

spalger commented Sep 8, 2018

spalger commented Sep 4, 2018 •

edited

Loading

spalger Sep 5, 2018 •

edited

Loading