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

[DX] Bump TS version to v4.1 #83397

Merged
merged 17 commits into from
Nov 24, 2020
Merged

[DX] Bump TS version to v4.1 #83397

merged 17 commits into from
Nov 24, 2020

Conversation

mshustov
Copy link
Contributor

@mshustov mshustov commented Nov 16, 2020

Summary

Release notes: https://devblogs.microsoft.com/typescript/announcing-typescript-4-1/
Breaking changes: https://devblogs.microsoft.com/typescript/announcing-typescript-4-1/#breaking-changes

I muted TS errors with @ts-expect-error in several places. Code owners, fix them in the branch or as the follow-up, please.

@@ -181,6 +181,7 @@ export abstract class Container<
// If we hit this, the panel was removed before the embeddable finished loading.
if (this.input.panels[id] === undefined) {
subscription.unsubscribe();
// @ts-expect-error undefined in not assignable to TEmbeddable | ErrorEmbeddable
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we need either to change untilEmbeddableLoaded signature or fix the code @elastic/kibana-app-arch

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe

reject(new Error('The panel was removed before the embeddable finished loading.'));

/cc @stacey-gammon

@@ -56,6 +56,7 @@ export class ContactCardEmbeddableFactory
<ContactCardInitializer
onCancel={() => {
modalSession.close();
// @ts-expect-error
Copy link
Contributor Author

Choose a reason for hiding this comment

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

undefined is not assignable to Partial<ContactCardEmbeddableInput> @elastic/kibana-app-arch

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe

reject(new Error('Cancelled!'));

@@ -23,6 +23,7 @@ export const Duration = React.memo<{
}>(({ contextId, eventId, fieldName, value }) => (
<DefaultDraggable
id={`duration-default-draggable-${contextId}-${eventId}-${fieldName}-${value}`}
// @ts-expect-error
Copy link
Contributor Author

Choose a reason for hiding this comment

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

name is not defined in the scope. It uses deprecated global var name from lib.dom.d.ts. See all other changes in security_solution plugin code. @elastic/siem

Copy link
Contributor

Choose a reason for hiding this comment

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

@restrry thanks so much for catching this.

Copy link
Contributor

Choose a reason for hiding this comment

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

We are now tracking work to fix this. Thanks!

{...reactRouterNavigate(history, linkToAddRepository(name))}
{...reactRouterNavigate(
history,
// @ts-expect-error
Copy link
Contributor Author

Choose a reason for hiding this comment

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

name is not defined in the scope. It uses deprecated global var name from lib.dom.d.ts. @elastic/es-ui

Copy link
Contributor

@cjcenizal cjcenizal Nov 20, 2020

Choose a reason for hiding this comment

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

I think you intended this comment to be on line 40 of hdfs_settings.tsx, above, correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

If so, could you clarify the error that's expected on this line?

Copy link
Contributor Author

@mshustov mshustov Nov 24, 2020

Choose a reason for hiding this comment

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

I think you intended this comment to be on line 40 of hdfs_settings.tsx, above, correct?

TS doesn't complain there, does it?
2020-11-24_08-56-41

If so, could you clarify the error that's expected on this line?

Could you elaborate? As state in the comment: name is not defined in the local scope and references a deprecated property name from the global scope.
2020-11-24_08-56-04

Copy link
Contributor

Choose a reason for hiding this comment

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

I've tested this locally and it looks like that name value can safely be removed. Would you mind removing the TS comment and the reference to the global "name" @restrry ? Otherwise I can do it as a follow up chore. Either way, no need to block on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would prefer the owners to remove this code, as some nuances may pop up.

@mshustov
Copy link
Contributor Author

@elastic/kibana-telemetry telemetry tests are failing again

@afharo
Copy link
Member

afharo commented Nov 16, 2020

@restrry, updating the snapshot should fix it, but I've created #83440 to permanently fix it for this one and any future updates.

@mshustov mshustov added chore Feature:Dependencies release_note:skip Skip the PR/issue when compiling release notes v7.11.0 v8.0.0 labels Nov 20, 2020
@mshustov mshustov marked this pull request as ready for review November 20, 2020 08:58
@mshustov mshustov requested review from a team as code owners November 20, 2020 08:58
Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

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

LGTM

@mshustov mshustov changed the title [WIP] Bump TS version to v4.1 [DX] Bump TS version to v4.1 Nov 20, 2020
@@ -7,7 +7,7 @@
<b>Signature:</b>

```typescript
SearchBar: React.ComponentClass<Pick<Pick<SearchBarProps, "query" | "isLoading" | "filters" | "onRefresh" | "onRefreshChange" | "refreshInterval" | "intl" | "indexPatterns" | "dataTestSubj" | "screenTitle" | "showQueryInput" | "showDatePicker" | "showAutoRefreshOnly" | "dateRangeFrom" | "dateRangeTo" | "isRefreshPaused" | "customSubmitButton" | "timeHistory" | "indicateNoData" | "onFiltersUpdated" | "trackUiMetric" | "savedQuery" | "showSaveQuery" | "onClearSavedQuery" | "showQueryBar" | "showFilterBar" | "onQueryChange" | "onQuerySubmit" | "onSaved" | "onSavedQueryUpdated">, "query" | "isLoading" | "filters" | "onRefresh" | "onRefreshChange" | "refreshInterval" | "indexPatterns" | "dataTestSubj" | "screenTitle" | "showQueryInput" | "showDatePicker" | "showAutoRefreshOnly" | "dateRangeFrom" | "dateRangeTo" | "isRefreshPaused" | "customSubmitButton" | "timeHistory" | "indicateNoData" | "onFiltersUpdated" | "trackUiMetric" | "savedQuery" | "showSaveQuery" | "onClearSavedQuery" | "showQueryBar" | "showFilterBar" | "onQueryChange" | "onQuerySubmit" | "onSaved" | "onSavedQueryUpdated">, any> & {
WrappedComponent: React.ComponentType<Pick<SearchBarProps, "query" | "isLoading" | "filters" | "onRefresh" | "onRefreshChange" | "refreshInterval" | "intl" | "indexPatterns" | "dataTestSubj" | "screenTitle" | "showQueryInput" | "showDatePicker" | "showAutoRefreshOnly" | "dateRangeFrom" | "dateRangeTo" | "isRefreshPaused" | "customSubmitButton" | "timeHistory" | "indicateNoData" | "onFiltersUpdated" | "trackUiMetric" | "savedQuery" | "showSaveQuery" | "onClearSavedQuery" | "showQueryBar" | "showFilterBar" | "onQueryChange" | "onQuerySubmit" | "onSaved" | "onSavedQueryUpdated"> & ReactIntl.InjectedIntlProps>;
SearchBar: React.ComponentClass<Pick<Pick<SearchBarProps, "query" | "isLoading" | "filters" | "intl" | "indexPatterns" | "dataTestSubj" | "refreshInterval" | "screenTitle" | "onRefresh" | "onRefreshChange" | "showQueryInput" | "showDatePicker" | "showAutoRefreshOnly" | "dateRangeFrom" | "dateRangeTo" | "isRefreshPaused" | "customSubmitButton" | "timeHistory" | "indicateNoData" | "onFiltersUpdated" | "trackUiMetric" | "savedQuery" | "showSaveQuery" | "onClearSavedQuery" | "showQueryBar" | "showFilterBar" | "onQueryChange" | "onQuerySubmit" | "onSaved" | "onSavedQueryUpdated">, "query" | "isLoading" | "filters" | "indexPatterns" | "dataTestSubj" | "refreshInterval" | "screenTitle" | "onRefresh" | "onRefreshChange" | "showQueryInput" | "showDatePicker" | "showAutoRefreshOnly" | "dateRangeFrom" | "dateRangeTo" | "isRefreshPaused" | "customSubmitButton" | "timeHistory" | "indicateNoData" | "onFiltersUpdated" | "trackUiMetric" | "savedQuery" | "showSaveQuery" | "onClearSavedQuery" | "showQueryBar" | "showFilterBar" | "onQueryChange" | "onQuerySubmit" | "onSaved" | "onSavedQueryUpdated">, any> & {
Copy link
Contributor Author

@mshustov mshustov Nov 20, 2020

Choose a reason for hiding this comment

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

@elastic/kibana-app-services would it possible to fix the problem #77678 (comment), please ?

Copy link
Contributor

@mikecote mikecote left a comment

Choose a reason for hiding this comment

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

Alerting team code changes LGTM

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

KibanaApp changes LGTM, code review only

Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

Operations: LGTM

Copy link
Contributor

@jfsiii jfsiii left a comment

Choose a reason for hiding this comment

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

Fleet changes LGTM

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.

infra changes LGTM

Copy link
Member

@dmlemeshko dmlemeshko left a comment

Choose a reason for hiding this comment

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

Kibana QA changes LGTM

Copy link
Contributor

@clintandrewhall clintandrewhall left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

Enterprise Search file changes look fine!

@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
advancedSettings 919.6KB 919.6KB -11.0B
beatsManagement 581.0KB 581.0KB +92.0B
console 982.1KB 982.2KB +99.0B
lens 936.9KB 936.9KB +20.0B
ml 5.2MB 5.2MB +99.0B
searchprofiler 690.7KB 690.8KB +99.0B
security 787.1KB 787.1KB +7.0B
securitySolution 7.9MB 7.9MB +9.0B
snapshotRestore 583.4KB 583.5KB +21.0B
transform 1021.2KB 1021.3KB +99.0B
triggersActionsUi 1.5MB 1.5MB +99.0B
watcher 955.2KB 955.3KB +99.0B
total +732.0B

Page load bundle

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

id before after diff
embeddable 221.5KB 221.6KB +9.0B

History

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

Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

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

Changes look good to me! Let me know what you decide RE the global name value @restrry

{...reactRouterNavigate(history, linkToAddRepository(name))}
{...reactRouterNavigate(
history,
// @ts-expect-error
Copy link
Contributor

Choose a reason for hiding this comment

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

I've tested this locally and it looks like that name value can safely be removed. Would you mind removing the TS comment and the reference to the global "name" @restrry ? Otherwise I can do it as a follow up chore. Either way, no need to block on this.

Copy link
Contributor

@oatkiller oatkiller left a comment

Choose a reason for hiding this comment

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

Thank you!

@mshustov mshustov merged commit 5ec6fe3 into elastic:master Nov 24, 2020
@mshustov mshustov deleted the ts-4-1-1 branch November 24, 2020 15:04
mshustov added a commit to mshustov/kibana that referenced this pull request Nov 24, 2020
* bump version to 4.1.1-rc

* fix code to run kbn bootstrap

* fix errors

* DO NOT MERGE. mute errors and ping teams to fix them

* Address EuiSelectableProps configuration in discover sidebar

* use explicit type for EuiSelectable

* update to ts v4.1.2

* fix ts error in EuiSelectable

* update docs

* update prettier with ts version support

* Revert "update prettier with ts version support"

This reverts commit 3de48db.

* address another new problem

Co-authored-by: Chandler Prall <chandler.prall@gmail.com>
# Conflicts:
#	test/functional/services/remote/remote.ts
mshustov added a commit that referenced this pull request Nov 24, 2020
* bump version to 4.1.1-rc

* fix code to run kbn bootstrap

* fix errors

* DO NOT MERGE. mute errors and ping teams to fix them

* Address EuiSelectableProps configuration in discover sidebar

* use explicit type for EuiSelectable

* update to ts v4.1.2

* fix ts error in EuiSelectable

* update docs

* update prettier with ts version support

* Revert "update prettier with ts version support"

This reverts commit 3de48db.

* address another new problem

Co-authored-by: Chandler Prall <chandler.prall@gmail.com>
# Conflicts:
#	test/functional/services/remote/remote.ts
rylnd added a commit to rylnd/kibana that referenced this pull request Nov 24, 2020
* master: (41 commits)
  [Maps] fix code-owners (elastic#84265)
  [@kbn/utils] Clean target before build (elastic#84253)
  [code coverage] collect for oss integration tests (elastic#83907)
  [APM] Use `asTransactionRate` consistently everywhere (elastic#84213)
  Attempt to fix incremental build error (elastic#84152)
  Unskip "Copy dashboards to space" (elastic#84115)
  Remove expressions.legacy from README (elastic#79681)
  Expression: Add render mode and use it for canvas interactivity (elastic#83559)
  [deb/rpm] Move systemd service to /usr/lib/systemd/system (elastic#83571)
  [Security Solution][Resolver] Allow a configurable entity_id field (elastic#81679)
  [ML] Space permision checks for job deletion (elastic#83871)
  [build] Provide ARM build of RE2 (elastic#84163)
  TSVB should use "histogram:maxBars" and "histogram:barTarget" settings for auto instead of a default 100 buckets (elastic#83628)
  [Workplace Search] Initial rendering of Org Sources (elastic#84164)
  update geckodriver to 0.28 (elastic#84085)
  Fix timelion vis escapes single quotes (elastic#84196)
  [Security Solution] Fix incorrect time for dns histogram (elastic#83532)
  [DX] Bump TS version to v4.1 (elastic#83397)
  [Security Solution] Add endpoint policy revision number (elastic#83982)
  [Fleet] Integration Policies List view (elastic#83634)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported chore Feature:Dependencies Feature:Embedding Embedding content via iFrame Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v7.11.0 v8.0.0
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet