Skip to content

Conversation

@mattkime
Copy link
Contributor

@mattkime mattkime commented Aug 10, 2020

Summary

Addressing #68020 - there will be a number of PRs that transition away from the field list extending the array class.

  • Introduce indexPattern.fields.getAll() and use where possible
  • Rename index_patterns/fields/fields.mocks.ts.ts => index_patterns/fields/fields.mocks.ts
  • FieldSpec - make count and scripted fields optional
  • use indexPattern.fields.getByName instead of filter where possible

@mattkime mattkime changed the title use fields.getAll where possible Index pattern field list - transition away from extending array - introduce and use getAll() Aug 11, 2020
@mattkime mattkime added Team:AppArch Feature:Data Views Data Views code and UI - index patterns before 8.0 v8.0.0 v7.10.0 release_note:skip Skip the PR/issue when compiling release notes labels Aug 11, 2020
@mattkime mattkime marked this pull request as ready for review August 11, 2020 02:48
@mattkime mattkime requested a review from a team as a code owner August 11, 2020 02:48
@mattkime mattkime requested a review from a team August 11, 2020 02:48
@mattkime mattkime requested review from a team as code owners August 11, 2020 02:48
@elasticmachine
Copy link
Contributor

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

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

async chunks size

id value diff baseline
discover 431.6KB +26.0B 431.6KB
indexPatternManagement 698.1KB +23.0B 698.1KB
total +49.0B

page load bundle size

id value diff baseline
data 1.4MB +141.0B 1.4MB
discover 229.0KB -87.0B 229.0KB
inputControlVis 295.7KB +9.0B 295.7KB
visTypeTimelion 711.9KB +27.0B 711.8KB
total +90.0B

History

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

Copy link
Contributor

@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

Copy link
Contributor

@lizozom lizozom left a comment

Choose a reason for hiding this comment

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

Overall LGTM.
Added one comment for your review.


export interface FieldSpec {
count: number;
count?: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the changes in src/plugins/data/common/index_patterns/fields/index_pattern_field.ts, it seems that both functions can't return undefined. Can it be that you don't have to make these optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lizozom I'm making them optional so that someone defining a field doesn't have the burden of providing them - the expected defaults are obvious. Its not a necessary change but I think its still helpful.

@mattkime mattkime merged commit 6c63b0d into elastic:master Aug 11, 2020
mattkime added a commit to mattkime/kibana that referenced this pull request Aug 11, 2020
…roduce and use getAll() (elastic#74718)

- Introduce `indexPattern.fields.getAll()` and use where possible
- Rename `index_patterns/fields/fields.mocks.ts.ts => index_patterns/fields/fields.mocks.ts`
- FieldSpec - make `count` and `scripted` fields optional
- use `indexPattern.fields.getByName` instead of filter where possible
# Conflicts:
#	src/plugins/index_pattern_management/public/components/field_editor/field_editor.test.tsx
mattkime added a commit that referenced this pull request Aug 11, 2020
…roduce and use getAll() (#74718) (#74756)

- Introduce `indexPattern.fields.getAll()` and use where possible
- Rename `index_patterns/fields/fields.mocks.ts.ts => index_patterns/fields/fields.mocks.ts`
- FieldSpec - make `count` and `scripted` fields optional
- use `indexPattern.fields.getByName` instead of filter where possible
gmmorris added a commit to gmmorris/kibana that referenced this pull request Aug 11, 2020
* master: (106 commits)
  [Functional Tests] Adds a wait time between setting the index pattern and the time field on TSVB (elastic#74736)
  [Lens] Add styling options for x and y axes on the settings popover (elastic#71829)
  [Maps] add initial location option that fits to data bounds (elastic#74583)
  theme function (elastic#73451)
  [data.ui.query] Write filters to query log from default editor. (elastic#74474)
  [data.search.SearchSource] Move some SearchSource dependencies to the server. (elastic#74607)
  [Canvas][tech-debt] Convert renderers (elastic#74134)
  [security solutions][lists] Adds end to end tests (elastic#74473)
  pluralized for occurrences vs occurrence (elastic#74564)
  Update links that pointed to CONTRIBUTING.md (elastic#74757)
  [Ingest pipelines] Implement tabs in processor flyout (elastic#74469)
  [Event log] Use Alerts client & Actions client when fetching these types of SOs (elastic#73257)
  Bump chalk to 4.1.0 (elastic#73397)
  Index pattern field list - transition away from extending array - introduce and use getAll() (elastic#74718)
  [SECURITY] Bugs css/inspect (elastic#74711)
  [telemetry] update README to downplay ui_metrics (elastic#74635)
  Fixed grammar (elastic#74725)
  [Telemetry][API Integration] size_in_bytes to be a number (elastic#74664)
  [ILM] Convert node details flyout to TS (elastic#73707)
  [Ingest Node Pipelines] Sentence-case processor names (elastic#74645)
  ...
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.

5 participants