Skip to content

[Lens] Get rid of giant union type#118848

Merged
flash1293 merged 8 commits intoelastic:mainfrom
flash1293:lens-union-type
Nov 19, 2021
Merged

[Lens] Get rid of giant union type#118848
flash1293 merged 8 commits intoelastic:mainfrom
flash1293:lens-union-type

Conversation

@flash1293
Copy link
Copy Markdown
Contributor

@flash1293 flash1293 commented Nov 17, 2021

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 Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// release_note:skip Skip the PR/issue when compiling release notes Feature:Lens auto-backport Deprecated - use backport:version if exact versions are needed v8.1.0 labels Nov 17, 2021
@flash1293
Copy link
Copy Markdown
Contributor Author

flash1293 commented Nov 17, 2021

@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
Copy Markdown
Contributor

elasticmachine commented Nov 17, 2021

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

patrykkopycinski
patrykkopycinski approved these changes Nov 17, 2021
View reviewed changes
Copy link
Copy Markdown
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

mbondyra
mbondyra approved these changes Nov 18, 2021
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Member

tylersmalley commented Nov 18, 2021

👏 ❤️ 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 added a commit to tylersmalley/kibana that referenced this pull request Nov 18, 2021
@tylersmalley
Committing change from elastic#118848
e333fdf 
Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
@tylersmalley tylersmalley mentioned this pull request Nov 18, 2021
Merged
2 tasks
@flash1293
Copy link
Copy Markdown
Contributor Author

flash1293 commented Nov 19, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Nov 19, 2021

expected head sha didn’t match current head ref.

dej611
dej611 approved these changes Nov 19, 2021
View reviewed changes
Copy link
Copy Markdown
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 ✅

peteharverson
peteharverson approved these changes Nov 19, 2021
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown

kibana-ci commented Nov 19, 2021

💚 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 PR does not require backporting label Nov 19, 2021
@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Nov 19, 2021

💔 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

@mbondyra mbondyra removed the auto-backport Deprecated - use backport:version if exact versions are needed label Nov 19, 2021
TinLe pushed a commit to TinLe/kibana that referenced this pull request Nov 20, 2021
@flash1293 @TinLe
[Lens] Get rid of giant union type (elastic#118848)
98c8232
dmlemeshko pushed a commit that referenced this pull request Nov 29, 2021
@flash1293 @dmlemeshko
[Lens] Get rid of giant union type (#118848)
96735f2
@mistic mistic mentioned this pull request May 31, 2023
Merged
mistic added a commit to mistic/kibana that referenced this pull request May 31, 2023
@mistic
[Lens] Get rid of giant union type (elastic#118848)
c97fad5 
(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
@mistic
[7.17] [Lens] Get rid of giant union type (#118848) (#158775)
5021ce5 
# 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":"https://github.com/elastic/kibana/pull/118848","mergeCommit":{"message":"[Lens]
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":"https://github.com/elastic/kibana/pull/118848","number":118848,"mergeCommit":{"message":"[Lens]
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

Reviewers

@dej611 dej611 dej611 approved these changes

@mbondyra mbondyra mbondyra approved these changes

@patrykkopycinski patrykkopycinski patrykkopycinski approved these changes

@peteharverson peteharverson peteharverson approved these changes

Assignees

No one assigned

Labels

backport:skip This PR does not require backporting Feature:Lens release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v7.17.11 v8.1.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@flash1293 @elasticmachine @tylersmalley @kibanamachine @kibana-ci @dej611 @mbondyra @patrykkopycinski @peteharverson