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

typescript indexPatterns service #39247

Merged
merged 23 commits into from Jul 30, 2019
Merged

typescript indexPatterns service #39247

merged 23 commits into from Jul 30, 2019

Conversation

ppisljar
Copy link
Member

@ppisljar ppisljar commented Jun 19, 2019 •
edited by lukeelmers

Summary

closes #39087 be removing config from this.

Typescript for index patterns service

Some minor changes to API were done:

  • cache is no longer exposed on indexPatterns, rather there is clearCache() method
  • delete method no longer exists on indexPatterns, rather there you should call indexPattern.destroy()

Follow-up items to investigate:

  • remove deprecated methods from src/legacy/ui/public/index_patterns/_field.ts
  • snapshots in create_index_pattern_wizard/components/step_time_field/__jest__/__snapshots__/step_time_field.test.js.snap were changed to isLoading=false. this feels like the correct behavior but need to confirm it isn't a problem.
  • error handling for src/legacy/ui/public/index_patterns/index_patterns.ts, specifically if savedObject.find encounters an error

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

Dev Docs

Minor changes to Index Patterns API

During our efforts to rewrite the index patterns service in TypeScript, we made a few minor changes to the API:

  • IndexPatterns.cache has been deprecated in favor of the new IndexPatterns.clearCache method.
  • IndexPatterns.delete has been deprecated. Instead, use the IndexPattern.destroy method on the specific index pattern you which to remove.
import { IndexPatterns, IndexPattern } from 'ui/index_patterns';
const indexPatterns = new IndexPatterns(config, savedObjectsClient);
const indexPattern = new IndexPattern('some-id', config, savedObjectsClient, ...etc);

// old
indexPatterns.cache.clear('some-id');
indexPatterns.delete(indexPattern);

// new
indexPatterns.clearCache('some-id'); // clears pattern matching specific ID from cache
indexPatterns.clearCache(); // clears everything
indexPattern.destroy(); // destroys pattern it is called on

@ppisljar ppisljar added v8.0.0 Team:AppArch release_note:skip Skip the PR/issue when compiling release notes v7.3.0 labels Jun 19, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

lukeelmers
lukeelmers reviewed Jun 21, 2019
View reviewed changes
Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

Still reviewing, but I pushed a couple updates to fix some of the issues, including that one with formatHits. Still seeing weirdness in the jest tests, related to mocking the TS files. Didn't have time to do a thorough investigation on that yet.

src/legacy/ui/public/index_patterns/_format_hit.ts Outdated

function convert(hit, val, fieldName) {
export function formatHitProvider(indexPattern: IndexPattern, defaultFormat: any) {
function convert(hit: any, val: any, fieldName: string) {
Copy link
Member

Choose a reason for hiding this comment

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

hit can be made slightly more restrictive here with Record<string, any>, so I'll update this too.

src/legacy/ui/public/index_patterns/_field_list.ts Outdated
export class FieldList extends IndexedArray<Field> {
// makes typescript work, but breaks kibana
//public byType: Record<string, Field[]> = {};
//public byName: Record<string, Field> = {};
Copy link
Member

Choose a reason for hiding this comment

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

It's because these should really be in the IndexedArray interface. I'll update this.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@nreese nreese mentioned this pull request Jun 26, 2019
Closed
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

lizozom
lizozom approved these changes Jul 28, 2019
View reviewed changes
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.

Still lots of work ahead of us, but looks good.
I would put a list of open, unaddressed issues \ questions in the issue's description.

Otherwise, LGTM.

jasonrhodes
jasonrhodes approved these changes Jul 29, 2019
View reviewed changes
Copy link
Member

@jasonrhodes jasonrhodes left a comment

Choose a reason for hiding this comment

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

LG(E)TM will revert my change in a follow up PR but this isn't broken so we're all good from my perspective for now

@lukeelmers
Merge branch 'master' of github.com:elastic/kibana into ts/indexPatterns
5e45264 
# Conflicts:
#	x-pack/legacy/plugins/ml/public/data_frame/pages/data_frame_new_pivot/components/source_index_preview/use_source_index_data.ts
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

peteharverson
peteharverson approved these changes Jul 30, 2019
View reviewed changes
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.

ML edits LGTM.
One quick question - does the type field in the StaticIndexPattern indicate if the index patten is a rollup type? We need this for the new ML job wizards.

@ppisljar
Copy link
Member Author

retest

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@lukeelmers lukeelmers merged commit ba8a453 into elastic:master Jul 30, 2019
App Arch New Platform Migration automation moved this from Code review to Needs backport Jul 30, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@lukeelmers lukeelmers mentioned this pull request Jul 30, 2019
Merged
@lukeelmers
Copy link
Member

does the type field in the StaticIndexPattern indicate if the index patten is a rollup type?

@peteharverson Exactly, for rollups you should be able to check indexPattern.type === 'rollup'

@lukeelmers
Copy link
Member

@peteharverson This also may be useful for reference:

kibana/x-pack/legacy/plugins/rollup/public/index_pattern_list/rollup_index_pattern_list_config.js

Lines 8 to 10 in 7ac8e4d

function isRollup(indexPattern) {
return indexPattern.type === 'rollup' || (indexPattern.get && indexPattern.get('type') === 'rollup');
}

@lukeelmers lukeelmers added release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. backport pending and removed release_note:skip Skip the PR/issue when compiling release notes labels Jul 30, 2019
@walterra walterra mentioned this pull request Aug 2, 2019
Closed
lukeelmers added a commit that referenced this pull request Aug 3, 2019
@lukeelmers
[7.x] Initial conversion of Index Patterns service to TypeScript. (#3…
ee3b21f 
…9247) (#42288)
@lukeelmers lukeelmers moved this from Needs backport to Done in App Arch New Platform Migration Aug 3, 2019
@lukeelmers lukeelmers mentioned this pull request Aug 5, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasonrhodes jasonrhodes jasonrhodes approved these changes

@lukeelmers lukeelmers lukeelmers approved these changes

@lizozom lizozom lizozom approved these changes

@peteharverson peteharverson peteharverson approved these changes

@streamich streamich Awaiting requested review from streamich

Assignees

@lukeelmers lukeelmers

Labels
backported Feature:NP Migration release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. v7.4.0 v8.0.0
Projects
No open projects
App Arch New Platform Migration
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Maps] angular RangeError: Maximum call stack size exceeded warning in console
6 participants
@ppisljar @elasticmachine @lukeelmers @jasonrhodes @lizozom @peteharverson