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.

Sign up for GitHub

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

Jump to bottom

TypeScript project references for infra plugin #90118

Merged
merged 13 commits into from
Feb 8, 2021
Merged

TypeScript project references for infra plugin #90118

merged 13 commits into from
Feb 8, 2021

Conversation

smith
Copy link
Contributor

@smith smith commented Feb 3, 2021 •
edited
Loading

Fixes #80995. References #81003. References #80508.

@smith smith added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.12.0 labels Feb 3, 2021
@smith smith requested a review from a team as a code owner February 3, 2021 05:39
@smith smith marked this pull request as draft February 3, 2021 05:39
smith
smith commented Feb 3, 2021
View reviewed changes
x-pack/test/tsconfig.json
@@ -9,67 +9,67 @@
"exclude": ["../typings/jest.d.ts"],
"references": [
{ "path": "../../src/core/tsconfig.json" },
{ "path": "../../src/plugins/telemetry_management_section/tsconfig.json" },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alphabetized these.

@smith smith mentioned this pull request Feb 3, 2021
Closed
15 tasks
@weltenwort
Copy link
Member

Oh wow, thanks a ton for jumping in, @smith ❤️

smith
smith commented Feb 3, 2021
View reviewed changes
x-pack/plugins/infra/common/inventory_models/types.ts
@@ -285,7 +285,7 @@ export const ESTopHitsAggRT = rt.type({
top_hits: rt.object,
});

interface SnapshotTermsWithAggregation {
export interface SnapshotTermsWithAggregation {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because of TS4023 (microsoft/TypeScript#5711) some additional interfaces need to be exported because we're emitting declarations:

x-pack/plugins/infra/server/routes/snapshot/lib/get_metrics_aggregations.ts:29:14 - error TS4023: Exported variable 'metricToAggregation' has or is using name 'SnapshotTermsWithAggregation' from external module "/Users/smith/Code/kibana/x-pack/plugins/infra/common/inventory_models/types" but cannot be named.

smith
smith commented Feb 3, 2021
View reviewed changes
x-pack/plugins/infra/public/components/document_title.tsx
@@ -47,19 +47,19 @@ const wrapWithSharedState = () => {
return null;
}

private getTitle(title: TitleProp) {
public getTitle(title: TitleProp) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is returning an anonymous class expression so private/protected are not permitted if we're emitting declarations. (TS4094/microsoft/TypeScript#30355)

smith
smith commented Feb 3, 2021
View reviewed changes
...k/plugins/infra/public/pages/metrics/inventory_view/components/waffle/custom_field_panel.tsx
@@ -26,7 +26,7 @@ const initialState = {

type State = Readonly<typeof initialState>;

export const CustomFieldPanel = class extends React.PureComponent<Props, State> {
export class CustomFieldPanel extends React.PureComponent<Props, State> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Export a named class instead of expression. (TS4094/microsoft/TypeScript#30355)

@smith
Copy link
Contributor Author

smith commented Feb 3, 2021

This is almost working but we're getting some EUI-related errors:

x-pack/plugins/infra/public/components/toolbar_panel.ts:10:14 - error TS4023: Exported variable 'ToolbarPanel' has or is using name 'Divlike' from external module "@elastic/eui/src/components/panel/panel" but cannot be named.

10 export const ToolbarPanel = euiStyled(EuiPanel).attrs(() => ({

In these errors the types are not exported from EUI. Looking at this to see if there's a workaround or if we need to export these from EUI.

@smith
Copy link
Contributor Author

smith commented Feb 3, 2021

Another error we're getting is related to the string enum export of Comparator:

x-pack/plugins/infra/common/http_api/log_alerts/chart_preview_data.ts:43:14 - error TS4023: Exported variable 'getLogAlertsChartPreviewDataAlertParamsSubsetRT' has or is using name 'Comparator' from external module "/Users/smith/Code/kibana/x-pack/plugins/infra/common/alerting/logs/log_threshold/types" but cannot be named.

43 export const getLogAlertsChartPreviewDataAlertParamsSubsetRT = rt.intersection([
                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

x-pack/plugins/infra/common/http_api/log_alerts/chart_preview_data.ts:58:14 - error TS4023: Exported variable 'getLogAlertsChartPreviewDataRequestPayloadRT' has or is using name 'Comparator' from external module "/Users/smith/Code/kibana/x-pack/plugins/infra/common/alerting/logs/log_threshold/types" but cannot be named.

58 export const getLogAlertsChartPreviewDataRequestPayloadRT = rt.type({
                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I'm not sure string enums get emitted as you would expect, so this may need to be converted into an object to work correctly.

@spalger
Copy link
Contributor

spalger commented Feb 3, 2021

jenkins test this

@smith
Copy link
Contributor Author

smith commented Feb 4, 2021

The issues with EUI are resolved. Working on fixing the problem with the Comparator enum.

@spalger
Copy link
Contributor

spalger commented Feb 4, 2021

jenkins test this

@weltenwort
Copy link
Member

@elasticmachine merge upstream

@smith smith marked this pull request as ready for review February 5, 2021 17:48
@smith
Copy link
Contributor Author

smith commented Feb 7, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
infra 1.9MB 1.9MB -240.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
infra 172.3KB 172.5KB +173.0B

History

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

@weltenwort weltenwort self-requested a review February 8, 2021 10:43
weltenwort
weltenwort approved these changes Feb 8, 2021
View reviewed changes
Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

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

Thanks again for driving this, @smith!

@smith smith merged commit e4794af into elastic:master Feb 8, 2021
@smith smith mentioned this pull request Feb 8, 2021
Merged
smith added a commit to smith/kibana that referenced this pull request Feb 8, 2021
@smith @weltenwort
TypeScript project references for infra plugin (elastic#90118)
4cdac26 
Co-authored-by: Felix Stürmer <[email protected]>
@smith smith mentioned this pull request Feb 8, 2021
Closed
smith added a commit that referenced this pull request Feb 8, 2021
@smith @weltenwort
TypeScript project references for infra plugin (#90118) (#90620)
ca0b1fe 
Co-authored-by: Felix Stürmer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weltenwort weltenwort weltenwort approved these changes

Assignees
No one assigned
Labels
release_note:skip Skip the PR/issue when compiling release notes v7.12.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Logs + Metrics UI] Migrate to TS project references
4 participants
@smith @weltenwort @spalger @kibanamachine