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

[NP] Remove IndexedArray usage in Schemas #61410

Merged
merged 13 commits into from
Apr 2, 2020
Merged

[NP] Remove IndexedArray usage in Schemas #61410

merged 13 commits into from
Apr 2, 2020

Conversation

maryia-lapata
Copy link
Contributor

Part of #60098.

Remove IndexedArray usage in Schemas

@maryia-lapata maryia-lapata added Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:NP Migration v7.8 labels Mar 26, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@maryia-lapata
Copy link
Contributor Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

merge conflict between base and head

@maryia-lapata
Merge remote-tracking branch 'remotes/kibana/master' into remove-inde…
f3fdb95 
…xed-array

# Conflicts:
#	src/legacy/core_plugins/visualizations/public/np_ready/public/vis.ts
@maryia-lapata
Revert default value
7e86148
@maryia-lapata
Update sidebar.tsx
307d727
@flash1293 flash1293 mentioned this pull request Mar 26, 2020
Closed
8 tasks
@maryia-lapata
Copy link
Contributor Author

@elasticmachine merge upstream

@maryia-lapata maryia-lapata marked this pull request as ready for review March 27, 2020 06:11
@maryia-lapata maryia-lapata requested a review from a team as a code owner March 27, 2020 06:11
maryia-lapata
maryia-lapata commented Mar 27, 2020
View reviewed changes
src/legacy/core_plugins/vis_default_editor/public/components/agg_group.tsx
@@ -70,7 +70,7 @@ function DefaultEditorAggGroup({
}: DefaultEditorAggGroupProps) {
const groupNameLabel = (search.aggs.aggGroupNamesMap() as any)[groupName];
// e.g. buckets can have no aggs
const schemaNames = getSchemasByGroup(schemas, groupName).map(s => s.name);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here schemas has already contained schemas by group from groupName. So we don't need to get them again.

sulemanof
sulemanof approved these changes Mar 31, 2020
View reviewed changes
Copy link
Contributor

@sulemanof sulemanof left a comment

Choose a reason for hiding this comment

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

Tested locally in Chrome, works as expected! 👍
I would only adjust some types as recommended below 👇

src/legacy/core_plugins/vis_default_editor/public/schemas.ts
@@ -46,8 +45,7 @@ export interface Schema {
}

export class Schemas {
// @ts-ignore
all: IndexedArray<Schema>;
all: Schema[] = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

I would specify metrics & buckets here also.
Basically, this class should implements the ISchemas interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Done.
Since now Schemas has the same properties as ISchemas has, I got rid of ISchemas.

Copy link
Contributor

@sulemanof sulemanof Apr 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

hmm, AFAIK ISchemas was intentionally created to use an interface instead of class instance (there were a lot of places with such an approach, so all of interfaces starts with I ).
@flash1293 am I right?

Copy link
Contributor

Choose a reason for hiding this comment

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

@stacey-gammon When I remember correctly you started to remove the types separate to actual class implementations in some places, right? Do you know what the current best practices around this are?

Copy link
Member

Choose a reason for hiding this comment

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

For awhile we (app arch) were moving toward an I prefix convention in these scenarios, but have since stopped doing this in favor of waiting for type-only imports which are available in TS 3.8. I believe this is the approach the platform team is taking too.

In the meantime, for the cases where we need a separate type in the interim, we have been inconsistent, using I* in some places but not in others.

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case I like the current solution, once TS 3.8 is merged, we can do a bulk change of the remaining places.

Copy link
Member

Choose a reason for hiding this comment

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

For those who want to follow along, here's the PR which is working toward upgrading TS in Kibana: #57774

Copy link
Contributor

Choose a reason for hiding this comment

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

Discussion here if you are interested on the I-prefix: #51674

src/legacy/core_plugins/vis_default_editor/public/schemas.ts Outdated Show resolved Hide resolved
flash1293
flash1293 approved these changes Mar 31, 2020
View reviewed changes
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Tested with various aggregation configurations (adding/removing) - everything seems to be there and working as intended - LGTM. Left some small nits about types in addition to Daniils remarks

src/legacy/core_plugins/vis_default_editor/public/components/agg_group.test.tsx Outdated Show resolved Hide resolved
src/legacy/core_plugins/vis_default_editor/public/components/agg_group.test.tsx Outdated Show resolved Hide resolved
@flash1293 flash1293 removed the v7.8 label Apr 1, 2020
@maryia-lapata
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@maryia-lapata maryia-lapata merged commit b4fd1eb into elastic:master Apr 2, 2020
@maryia-lapata maryia-lapata deleted the remove-indexed-array branch April 2, 2020 09:31
@maryia-lapata maryia-lapata mentioned this pull request Apr 2, 2020
Merged
maryia-lapata added a commit that referenced this pull request Apr 2, 2020
@maryia-lapata @elasticmachine
[NP] Remove IndexedArray usage in Schemas (#61410) (#62279)
16bc6d6 
* Remove IndexedArray usage from schemas.ts

* Update unit tests

* Revert default value

* Update sidebar.tsx

* Updated TS

* Fix code review comments

* Remove ISchemas

* Revert ISchemas

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Apr 2, 2020
@gmmorris
Merge branch 'master' into event-log/query-support
3eada98 
* master:
  Switch to embeddable factory interface with optional override (elastic#61165)
  fix text error in diagrams (elastic#62101)
  [Index management] Prepare support Index template V2 format (elastic#61588)
  Updates dashboard images (elastic#62011)
  [Maps] remove MapBounds type (elastic#62332)
  [Uptime] Convert anomaly Jobs name to lowercase to comply with… (elastic#62293)
  Make d3 place nicely with object values (elastic#62004)
  EMT-287: update schema with elastic agent id (elastic#62252)
  [Maps] fix replaceLayerList to handle case where map is not intialized (elastic#62202)
  Remove support for deprecated xpack.telemetry configurations (elastic#51142)
  [Uptime] Remove static constant for index name completely (elastic#62256)
  [APM] E2E: install dependencies for vanilla workspaces (elastic#62178)
  [backport] Bump to 5.1.3 (elastic#62286)
  Show server name in Remote Cluster detail panel (elastic#62250)
  Rename some alert types (elastic#61693)
  changing duration type to ms, s, m (elastic#62265)
  [ML] Clear Kibana index pattern cache on creation or form reset. (elastic#62184)
  Move `src/legacy/server/index_patterns` to data plugin (server) (Remove step) (elastic#61618)
  [NP] Remove IndexedArray usage in Schemas (elastic#61410)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sulemanof sulemanof sulemanof approved these changes

@stacey-gammon stacey-gammon stacey-gammon left review comments

@lukeelmers lukeelmers lukeelmers left review comments

@flash1293 flash1293 flash1293 approved these changes

Assignees
No one assigned
Labels
backported Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.8.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@maryia-lapata @elasticmachine @kibanamachine @flash1293 @lukeelmers @stacey-gammon @sulemanof