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

Index pattern save => Index pattern service #76706

Merged
merged 16 commits into from
Sep 8, 2020
Merged

Index pattern save => Index pattern service #76706

merged 16 commits into from
Sep 8, 2020

Conversation

mattkime
Copy link
Contributor

@mattkime mattkime commented Sep 3, 2020 •
edited
Loading

Summary

  • IndexPattern.save=> IndexPatternsService.save
  • IndexPatternService is passed into IndexPattern instances as a dependency. This is a temporary compromise while refactoring that allows smaller PRs.

Checklist

For maintainers

Dev docs

  • IndexPattern.save has been replaced with IndexPatternsService.save

@mattkime mattkime changed the title partial progress Index pattern save refactor Sep 3, 2020
@mattkime mattkime changed the title Index pattern save refactor Index pattern save => Index pattern service Sep 6, 2020
@mattkime
Copy link
Contributor Author

mattkime commented Sep 6, 2020

@elasticmachine merge upstream

@mattkime mattkime marked this pull request as ready for review September 7, 2020 00:15
@mattkime mattkime requested review from a team as code owners September 7, 2020 00:15
@mattkime mattkime added Feature:Data Views Data Views code and UI - index patterns before 8.0 Team:AppArch labels Sep 7, 2020
@elasticmachine
Copy link
Contributor

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

@mattkime mattkime added v8.0.0 v7.10.0 release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. labels Sep 7, 2020
ppisljar
ppisljar approved these changes Sep 8, 2020
View reviewed changes
Copy link
Member

@ppisljar ppisljar 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

mshustov
mshustov approved these changes Sep 8, 2020
View reviewed changes
Copy link
Contributor

@mshustov mshustov left a comment

Choose a reason for hiding this comment

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

Approved to unblock merge request. src/core/server/server.api.md will be updated when you update branch with the latest master

mshustov
mshustov reviewed Sep 8, 2020
View reviewed changes
docs/development/plugins/data/public/kibana-plugin-plugins-data-public.querystringinput.md
@@ -7,5 +7,5 @@
<b>Signature:</b>

```typescript
QueryStringInput: React.FC<Pick<Props, "placeholder" | "onBlur" | "onChange" | "onSubmit" | "query" | "size" | "indexPatterns" | "prepend" | "screenTitle" | "dataTestSubj" | "disableAutoFocus" | "persistedLog" | "bubbleSubmitEvent" | "languageSwitcherPopoverAnchorPosition" | "onChangeQueryInputFocus">>
QueryStringInput: React.FC<Pick<Props, "query" | "prepend" | "size" | "className" | "placeholder" | "onChange" | "onBlur" | "onSubmit" | "indexPatterns" | "dataTestSubj" | "screenTitle" | "disableAutoFocus" | "persistedLog" | "bubbleSubmitEvent" | "languageSwitcherPopoverAnchorPosition" | "onChangeQueryInputFocus">>
Copy link
Contributor

Choose a reason for hiding this comment

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

do you have the same problem with latest master? #76890

Copy link
Contributor

@mshustov mshustov Sep 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

note that this type in the current form doesn't provide a lot of value and it's better to declare it explicitly microsoft/rushstack#1958 (comment) instead of relying on an inferred type

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 changed export const QueryStringInput = withKibana(QueryStringInputUI); to export const QueryStringInput = withKibana<Props>(QueryStringInputUI); with no change in the resulting docs.

Copy link
Contributor

@mshustov mshustov Sep 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Right, because withKibana is inferring the result type with Omit.
Before:

const QueryStringInput: React.FC<Pick<Pick<Props, "query" | "prepend" ....>

After:

const QueryStringInput: React.FC<Pick<Props, "query" | "prepend" ....>

The main point that the generated document doesn't provide any useful information about the inferred interface. It's hard to say what is a type of query property, for example.
For comparison: the output with an explicit QueryStringInputProps interface #77054

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, right right.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

async chunks size

id value diff baseline
indexPatternManagement 807.4KB +860.0B 806.5KB

page load bundle size

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

History

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

@mattkime mattkime merged commit 842bcf4 into elastic:master Sep 8, 2020
@mattkime mattkime mentioned this pull request Sep 8, 2020
Merged
mattkime added a commit to mattkime/kibana that referenced this pull request Sep 8, 2020
@mattkime
Index pattern save => Index pattern service (elastic#76706)
cd1e8b2 
* Index pattern save => Index pattern service
mattkime added a commit that referenced this pull request Sep 8, 2020
@mattkime
Index pattern save => Index pattern service (#76706) (#76968)
0a67c7f 
* Index pattern save => Index pattern service
@mshustov mshustov mentioned this pull request Sep 23, 2020
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mshustov mshustov mshustov approved these changes

@ppisljar ppisljar ppisljar approved these changes

Assignees
No one assigned
Labels
Feature:Data Views Data Views code and UI - index patterns before 8.0 release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. v7.10.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mattkime @elasticmachine @kibanamachine @mshustov @ppisljar