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

Index pattern class cleanup - remove _.apply and any instance #76004

Merged
merged 9 commits into from
Aug 27, 2020

Conversation

mattkime
Copy link
Contributor

@mattkime mattkime commented Aug 26, 2020

Summary

Index pattern class cleanup - remove _.apply and any instance. Leaving [key: string]: any; means indexPatternInstance.somethingCrazy doesn't error in typescript.

Checklist

@mattkime mattkime changed the title no _.apply Index pattern class cleanup - remove _.apply and any instance Aug 27, 2020
@mattkime mattkime marked this pull request as ready for review August 27, 2020 04:58
@mattkime mattkime requested a review from a team as a code owner August 27, 2020 04:58
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

@mattkime mattkime added Feature:Data Views Data Views code and UI - index patterns before 8.0 v8.0.0 v7.10.0 labels Aug 27, 2020

const serverChangedKeys: string[] = [];
Object.entries(updatedBody).forEach(([key, value]) => {
// @ts-ignore
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 think these two ts-ignore statements are an acceptable compromise for now. It would be nice to do a deeper refactoring on this code first.

@mattkime mattkime added the release_note:skip Skip the PR/issue when compiling release notes label Aug 27, 2020
@elastic elastic deleted a comment from kibanamachine Aug 27, 2020
Copy link
Contributor

@streamich streamich 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, but I see there is one functional test that needs fixing.

Comment on lines 530 to 531
// @ts-ignore
if (value !== body[key] && value !== this.originalBody[key]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Better use casting:

Suggested change
// @ts-ignore
if (value !== body[key] && value !== this.originalBody[key]) {
if (value !== (body as any)[key] && value !== this.originalBody[key]) {

Copy link
Contributor

Choose a reason for hiding this comment

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

If you disable TypeScript, there is absolutely no error checking for that line:

image

If you cast, you still see all the errors:

image

Comment on lines 558 to 559
// @ts-ignore
this[key] = samePattern[key];
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// @ts-ignore
this[key] = samePattern[key];
(this as any)[key] = (samePattern as any)[key];

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

page load bundle size

id value diff baseline
data 1.4MB +1.4KB 1.4MB

History

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

@mattkime mattkime merged commit 3541e77 into elastic:master Aug 27, 2020
mattkime added a commit to mattkime/kibana that referenced this pull request Aug 27, 2020
…tic#76004)

Index pattern class cleanup - remove _.apply and `any` instance
# Conflicts:
#	docs/development/plugins/data/public/kibana-plugin-plugins-data-public.indexpattern.intervalname.md
#	docs/development/plugins/data/public/kibana-plugin-plugins-data-public.indexpattern.md
#	src/plugins/data/common/index_patterns/index_patterns/index_pattern.ts
#	src/plugins/data/public/public.api.md
mattkime added a commit that referenced this pull request Aug 27, 2020
…#76004) (#76109)

* Index pattern class cleanup - remove _.apply and `any` instance  (#76004)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Data Views Data Views code and UI - index patterns before 8.0 release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants