[new-platform] Explicitly define core setup types #34817

rudolf · 2019-04-09T20:39:56Z

Summary

Addresses the following services from #34416:

BasePathSetup
I18nSetup
InjectedMetadataSetup
FatalErrorsSetup

Instead of trying to address all services in a single big PR, I'm breaking this up into smaller PR's to minimize the potential for merge conflicts.

Checklist

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

~~[ ] This was checked for cross-browser compatibility, including a check against IE11~~
~~[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~[ ] Documentation was added for features that require explanation or tutorials~~
~~[ ] Unit or functional tests were updated or added to match the most common scenarios~~
~~This was checked for keyboard-only and screenreader accessibility~~

For maintainers

~~[ ] This was checked for breaking API changes and was labeled appropriately~~
~~[ ] This includes a feature addition or change that requires a release note and was labeled appropriately~~

elasticmachine · 2019-04-09T20:40:27Z

Pinging @elastic/kibana-platform

elasticmachine · 2019-04-09T21:27:10Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…up-types

elasticmachine · 2019-04-10T09:44:38Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-10T12:52:50Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

eliperelman

Functionally LGTM, just had some nits surrounding formatting consistency.

eliperelman · 2019-04-10T15:00:17Z

docs/development/core/public/kibana-plugin-public.basepathsetup.addtopath.md

+<b>Returns:</b>
+
+`string`
+


nit: Remove this extra trailing line.

These docs are generated by api-documenter (I just bumped the version to include a PR I submitted to add a "do not edit header" so that this is clearer). At some point we might fork api-documenter to customize the formatting to our needs since I agree that the formatting like 4-space indentation is different from our standards.

eliperelman · 2019-04-10T15:00:22Z

docs/development/core/public/kibana-plugin-public.basepathsetup.get.md

+`string`
+
+The basePath as defined by the server
+


nit: Remove this extra trailing line.

eliperelman · 2019-04-10T15:00:46Z

docs/development/core/public/kibana-plugin-public.basepathsetup.md

+| [addToPath(path)](./kibana-plugin-public.basepathsetup.addtopath.md) | Add the current basePath to a path string. |
+| [get()](./kibana-plugin-public.basepathsetup.get.md) | Get the basePath as defined by the server |
+| [removeFromPath(path)](./kibana-plugin-public.basepathsetup.removefrompath.md) | Remove the basePath from a path that starts with it |
+


nit: Remove this extra trailing line.

eliperelman · 2019-04-10T15:00:56Z

docs/development/core/public/kibana-plugin-public.basepathsetup.removefrompath.md

+<b>Returns:</b>
+
+`string`
+


nit: Remove this extra trailing line.

eliperelman · 2019-04-10T15:05:16Z

docs/development/core/public/kibana-plugin-public.i18nsetup.context.md

+
+```typescript
+Context: ({ children }: {
+ children: React.ReactNode;


nit: indentation consistency.

Suggested change

children: React.ReactNode;

children: React.ReactNode;

this file is autogenerated. the problem should be fixed in https:/Microsoft/web-build-tools/tree/master/apps/api-documenter
@skaapgif did you create an issue for this problem or I should?

eliperelman · 2019-04-10T15:10:29Z

docs/development/core/public/kibana-plugin-public.injectedmetadatasetup.getplugins.md

+
+```typescript
+getPlugins: () => Array<{
+ id: string;


nit: indentation consistency.

Suggested change

id: string;

id: string;

eliperelman · 2019-04-10T15:10:46Z

docs/development/core/public/kibana-plugin-public.injectedmetadatasetup.getplugins.md

+```typescript
+getPlugins: () => Array<{
+ id: string;
+ plugin: DiscoveredPlugin;


nit: indentation consistency.

Suggested change

plugin: DiscoveredPlugin;

plugin: DiscoveredPlugin;

eliperelman · 2019-04-10T15:11:07Z

docs/development/core/public/kibana-plugin-public.injectedmetadatasetup.getplugins.md

+getPlugins: () => Array<{
+ id: string;
+ plugin: DiscoveredPlugin;
+ }>;


nit: indentation consistency.

Suggested change

}>;

}>;

eliperelman · 2019-04-10T15:11:36Z

docs/development/core/public/kibana-plugin-public.injectedmetadatasetup.md

+| [getKibanaVersion](./kibana-plugin-public.injectedmetadatasetup.getkibanaversion.md) | <code>() =&gt; string</code> | |
+| [getLegacyMetadata](./kibana-plugin-public.injectedmetadatasetup.getlegacymetadata.md) | <code>() =&gt; {`<p/>` app: unknown;`<p/>` translations: unknown;`<p/>` bundleId: string;`<p/>` nav: unknown;`<p/>` version: string;`<p/>` branch: string;`<p/>` buildNum: number;`<p/>` buildSha: string;`<p/>` basePath: string;`<p/>` serverName: string;`<p/>` devMode: boolean;`<p/>` uiSettings: {`<p/>` defaults: UiSettingsState;`<p/>` user?: UiSettingsState &#124; undefined;`<p/>` };`<p/>` }</code> | |
+| [getPlugins](./kibana-plugin-public.injectedmetadatasetup.getplugins.md) | <code>() =&gt; Array&lt;{`<p/>` id: string;`<p/>` plugin: DiscoveredPlugin;`<p/>` }&gt;</code> | An array of frontend plugins in topological order. |
+


nit: remove extra trailing line for consistent file endings.

eliperelman · 2019-04-10T15:14:09Z

src/core/public/i18n/i18n_service.tsx

-/** @public */
-export type I18nSetup = ReturnType<I18nService['setup']>;
+/**
+ * I18nSetup.Context is required by any localizable React component from \@kbn/i18n and \@elastic/eui packages


Can I ask why moving this comment now has forward slashes in front of @kbn and @elastic?

The TSDoc standard uses @... for tags, so anytime an '@' is used it needs to be escaped. Before these comments weren't exported as part of the type system, so they weren't interpreted by TSDoc/api-exporter, so it wasn't an issue.

mshustov · 2019-04-10T15:25:17Z

docs/development/core/public/kibana-plugin-public.basepathsetup.removefrompath.md

+
+## BasePathSetup.removeFromPath() method
+
+Remove the basePath from a path that starts with it


optional: if starts with it

mshustov · 2019-04-10T15:28:06Z

docs/development/core/public/kibana-plugin-public.i18nsetup.context.md

+
+```typescript
+Context: ({ children }: {
+ children: React.ReactNode;


this file is autogenerated. the problem should be fixed in https:/Microsoft/web-build-tools/tree/master/apps/api-documenter
@skaapgif did you create an issue for this problem or I should?

mshustov · 2019-04-10T15:33:08Z

src/core/public/fatal_errors/fatal_errors_service.tsx

@@ -56,7 +78,7 @@ export class FatalErrorsService {
 });
 }

- public add = (error: Error | string, source?: string) => {
+ public add: FatalErrorsSetup['add'] = (error, source?) => {


private. no-one should call it on an instance

elasticmachine · 2019-04-10T15:38:58Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

rudolf · 2019-04-10T18:30:30Z

@restrry for some reason github won't let me reply to this comment

this file is autogenerated. the problem should be fixed in https:/Microsoft/web-build-tools/tree/master/apps/api-documenter
@skaapgif did you create an issue for this problem or I should?

I've created microsoft/rushstack#1215 which has been merged and released as [email protected]. I haven't yet updated Kibana's version, but I can do it as part of this PR 👍

elasticmachine · 2019-04-10T19:38:28Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

mshustov · 2019-04-11T06:22:07Z

@skaapgif what about other points in #32148 (review) ? do they have appropriate issues in https:/Microsoft/web-build-tools/ repo? should we create them?

rudolf · 2019-04-11T12:39:34Z

@restrry There's been quite a lot of changes in api-extractor, so I wanted to just make sure we're on the latest version of both extractor and documenter before creating issues. I've bumped api-extractor here: #34925

mshustov · 2019-04-11T13:17:48Z

@skaapgif yeah, problems are still present 😞

elasticmachine · 2019-04-15T09:29:15Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-16T10:42:59Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

Formatting issues are due to upstream dependency, I've created issues for these and will follow-up once there's progress.

mshustov

I'd change method definition https:/elastic/kibana/pull/34817/files#r274024421 but overall everything is 🆗

elasticmachine · 2019-04-16T18:54:12Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

This reverts commit fe9e6e6.

rudolf · 2019-04-16T20:06:09Z

I'd change method definition https:/elastic/kibana/pull/34817/files#r274024421 but overall everything is 🆗

I totally agree with your reasoning here, but I'll open a new PR for this.

elasticmachine · 2019-04-16T20:58:06Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* Explicitly define BasePathSetup type * Explicitly define I18nSetup type * Explicitly define InjectedMetadataSetup type * Explicitly define FatalErrorSetup type * Fix lint errors * Fix BasePathSetup types * Fix lint errors * Clarify basePath add() docs * Upgrade api-documenter for 'do not edit' header * Use @link references on CoreSetup to improve doc navigation * Add comment that links to upstream api-documenter issue * PR Comments: make FatalErrorsService.add() private * Revert "PR Comments: make FatalErrorsService.add() private" This reverts commit fe9e6e6.

rudolf added 3 commits April 9, 2019 15:49

Explicitly define BasePathSetup type

37c37d1

Explicitly define I18nSetup type

f60a547

Explicitly define InjectedMetadataSetup type

fb481d2

rudolf added Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc non-issue Indicates to automation that a pull request should not appear in the release notes v7.2.0 v8.0.0 labels Apr 9, 2019

rudolf mentioned this pull request Apr 9, 2019

[new platform] Explicitly define service setup types #34416

Closed

8 tasks

Explicitly define FatalErrorSetup type

483f901

rudolf changed the title ~~Core explicit setup types~~ Explicitly define core setup types Apr 9, 2019

rudolf changed the title ~~Explicitly define core setup types~~ [new-platform] Explicitly define core setup types Apr 9, 2019

Merge remote-tracking branch 'upstream/master' into core-explicit-set…

4157d31

…up-types

rudolf marked this pull request as ready for review April 10, 2019 09:00

rudolf requested a review from a team as a code owner April 10, 2019 09:00

rudolf added the review label Apr 10, 2019

Fix lint errors

4ad80a7

Fix BasePathSetup types

f3d8fd7

eliperelman previously requested changes Apr 10, 2019

View reviewed changes

mshustov reviewed Apr 10, 2019

View reviewed changes

rudolf added 3 commits April 10, 2019 20:45

Fix lint errors

f5b06a3

Clarify basePath add() docs

635a933

Upgrade api-documenter for 'do not edit' header

9896977

Merge branch 'master' into core-explicit-setup-types

b2ea641

rudolf mentioned this pull request Apr 16, 2019

Generate core API docs from TSDoc comments #32148

Merged

9 tasks

rudolf added 2 commits April 16, 2019 11:46

Use @link references on CoreSetup to improve doc navigation

cb0818b

Add comment that links to upstream api-documenter issue

0c10708

rudolf requested a review from mshustov April 16, 2019 12:07

mshustov approved these changes Apr 16, 2019

View reviewed changes

PR Comments: make FatalErrorsService.add() private

fe9e6e6

Revert "PR Comments: make FatalErrorsService.add() private"

100d447

This reverts commit fe9e6e6.

rudolf merged commit 764bf25 into elastic:master Apr 16, 2019

rudolf deleted the core-explicit-setup-types branch April 16, 2019 20:19

rudolf mentioned this pull request Apr 17, 2019

[7.x] [new-platform] Explicitly define core setup types (#34817) #35209

Merged


		## BasePathSetup.removeFromPath() method

		Remove the basePath from a path that starts with it

		<b>Returns:</b>

		`string`

		<b>Returns:</b>

		`string`

[new-platform] Explicitly define core setup types #34817

[new-platform] Explicitly define core setup types #34817

Conversation

rudolf commented Apr 9, 2019 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Apr 9, 2019

elasticmachine commented Apr 9, 2019

💔 Build Failed

elasticmachine commented Apr 10, 2019

💔 Build Failed

elasticmachine commented Apr 10, 2019

💔 Build Failed

eliperelman 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

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

Choose a reason for hiding this comment

elasticmachine commented Apr 10, 2019

💔 Build Failed

rudolf commented Apr 10, 2019

elasticmachine commented Apr 10, 2019

💚 Build Succeeded

mshustov commented Apr 11, 2019 • edited Loading

rudolf commented Apr 11, 2019

mshustov commented Apr 11, 2019

elasticmachine commented Apr 15, 2019

💔 Build Failed

elasticmachine commented Apr 16, 2019

💚 Build Succeeded

mshustov left a comment

Choose a reason for hiding this comment

elasticmachine commented Apr 16, 2019

💔 Build Failed

rudolf commented Apr 16, 2019

elasticmachine commented Apr 16, 2019

💚 Build Succeeded

rudolf commented Apr 9, 2019 •

edited

Loading

mshustov commented Apr 11, 2019 •

edited

Loading