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

[Lens] Get rid of giant union type #118848

Merged
merged 8 commits into from
Nov 19, 2021
Merged

Conversation

flash1293
Copy link
Contributor

Lens has been using a large union type of all possible column types in a lot of places. However, typescript is struggling to compute it and it's currently blocking a TS upgrade as it's "too complex to analyze".

This PR gets rid of the union type and replaces is with a generic GenericIndexPatternColumn type which doesn't allow to access the specific params of the various columns directly. In places where this is necessary, the newly introduced helper isColumnOfType<SpecificColumnType>('specificOperationType', column) can be used as a type guard.

Most of the changes in this PR are caused by tests because we can't just provide matching params now - instead, the literal object has to be casted to the right type manually.

This change shouldn't hurt our type safety much in practice - the biggest impact is that the explicit isColumnOfType has to be used now instead of implicit type guards and column.operationType checks.

@flash1293 flash1293 added Team:Visualizations Visualization editors, elastic-charts and infrastructure release_note:skip Skip the PR/issue when compiling release notes Feature:Lens auto-backport Deprecated: Automatically backport this PR after it's merged v8.1.0 labels Nov 17, 2021
@flash1293
Copy link
Contributor Author

@elasticmachine merge upstream

@flash1293 flash1293 marked this pull request as ready for review November 17, 2021 17:54
@flash1293 flash1293 requested review from a team as code owners November 17, 2021 17:54
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-vis-editors @elastic/kibana-vis-editors-external (Team:VisEditors)

Copy link
Contributor

@patrykkopycinski patrykkopycinski left a comment

Choose a reason for hiding this comment

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

Asset management LGTM

Copy link
Contributor

@mbondyra mbondyra left a comment

Choose a reason for hiding this comment

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

Code LGTM, I didn't test manually but I think test coverage is enough here 🆗

@tylersmalley
Copy link
Contributor

👏 ❤️ verified it resolves the issue in the TS bump.

% node scripts/type_check.js --project x-pack/plugins/lens/tsconfig.json 
 info Building TypeScript projects refs for x-pack/plugins/lens/tsconfig.json...
 info [tsc] > node_modules/typescript/bin/tsc -b x-pack/plugins/lens/tsconfig.json --pretty
 info [tsc] exited with 0 after 30.2 seconds
 info running type check in 1 projects
 succ x-pack/plugins/lens/tsconfig.json

tylersmalley pushed a commit to tylersmalley/kibana that referenced this pull request Nov 18, 2021
Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
@tylersmalley tylersmalley mentioned this pull request Nov 18, 2021
2 tasks
@flash1293
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

expected head sha didn’t match current head ref.

Copy link
Contributor

@dej611 dej611 left a comment

Choose a reason for hiding this comment

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

Code review ✅

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Data visualizer type edits LGTM

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
lens 239 241 +2

Async chunks

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

id before after diff
lens 962.1KB 962.2KB +91.0B

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
lens 24 23 -1
Unknown metric groups

API count

id before after diff
lens 257 258 +1

History

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

@flash1293 flash1293 merged commit 8398d53 into elastic:main Nov 19, 2021
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Nov 19, 2021
@kibanamachine
Copy link
Contributor

💔 Backport failed

The backport operation could not be completed due to the following error:
There are no branches to backport to. Aborting.

The backport PRs will be merged automatically after passing CI.

To backport manually run:
node scripts/backport --pr 118848

ersin-erdal pushed a commit to ersin-erdal/kibana that referenced this pull request Nov 19, 2021
fkanout pushed a commit to fkanout/kibana that referenced this pull request Nov 19, 2021
@mbondyra mbondyra removed the auto-backport Deprecated: Automatically backport this PR after it's merged label Nov 19, 2021
TinLe pushed a commit to TinLe/kibana that referenced this pull request Nov 20, 2021
gbamparop pushed a commit to gbamparop/kibana that referenced this pull request Jan 12, 2022
mistic pushed a commit to mistic/kibana that referenced this pull request May 31, 2023
(cherry picked from commit 8398d53)

# Conflicts:
#	x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx
#	x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/index.ts
#	x-pack/plugins/lens/public/indexpattern_datasource/utils.ts
mistic added a commit to mistic/kibana that referenced this pull request May 31, 2023
(cherry picked from commit 8398d53)

# Conflicts:
#	x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx
#	x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/index.ts
#	x-pack/plugins/lens/public/indexpattern_datasource/utils.ts
mistic added a commit that referenced this pull request May 31, 2023
# Backport

This will backport the following commits from `main` to `7.17`:
- [[Lens] Get rid of giant union type
(#118848)](#118848)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Joe
Reuter","email":"johannes.reuter@elastic.co"},"sourceCommit":{"committedDate":"2021-11-19T12:00:35Z","message":"[Lens]
Get rid of giant union type
(#118848)","sha":"8398d53da4542c0673861d07ce565515318114ea","branchLabelMapping":{"^v8.1.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Team:Visualizations","release_note:skip","Feature:Lens","backport:skip","v8.1.0"],"number":118848,"url":"#118848
Get rid of giant union type
(#118848)","sha":"8398d53da4542c0673861d07ce565515318114ea"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.1.0","labelRegex":"^v8.1.0$","isSourceBranch":true,"state":"MERGED","url":"#118848
Get rid of giant union type
(#118848)","sha":"8398d53da4542c0673861d07ce565515318114ea"}}]}]
BACKPORT-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Lens release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.17.11 v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants