Skip to content

[NP] Remove IndexedArray usage in Schemas #61410

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

Merged
merged 13 commits into from
Apr 2, 2020
Merged

[NP] Remove IndexedArray usage in Schemas #61410

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
7948112
Remove IndexedArray usage from schemas.ts
maryia-lapata Mar 26, 2020
e89b8a4
Update unit tests
maryia-lapata Mar 26, 2020
f3fdb95
Merge remote-tracking branch 'remotes/kibana/master' into remove-inde…
maryia-lapata Mar 26, 2020
7e86148
Revert default value
maryia-lapata Mar 26, 2020
307d727
Update sidebar.tsx
maryia-lapata Mar 26, 2020
0b46f76
Merge branch 'master' into remove-indexed-array
elasticmachine Mar 26, 2020
dfdd6be
Merge remote-tracking branch 'kibana/master' into remove-indexed-array
maryia-lapata Mar 30, 2020
b495d47
Updated TS
maryia-lapata Mar 30, 2020
cc0afc3
Merge remote-tracking branch 'kibana/master' into remove-indexed-array
maryia-lapata Apr 1, 2020
97a875d
Fix code review comments
maryia-lapata Apr 1, 2020
39f1b1d
Remove ISchemas
maryia-lapata Apr 1, 2020
3084e2d
Merge branch 'master' into remove-indexed-array
elasticmachine Apr 2, 2020
b95165b
Revert ISchemas
maryia-lapata Apr 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 18 additions & 13 deletions src/legacy/core_plugins/vis_default_editor/public/components/agg_group.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import { IAggConfigs, IAggConfig } from 'src/plugins/data/public';
import { DefaultEditorAggGroup, DefaultEditorAggGroupProps } from './agg_group';
import { DefaultEditorAgg } from './agg';
import { DefaultEditorAggAdd } from './agg_add';
import { Schema } from '../schemas';
import { ISchemas, Schemas } from '../schemas';
import { EditorVisState } from './sidebar/state/reducers';

jest.mock('@elastic/eui', () => ({
Expand All @@ -47,6 +47,7 @@ jest.mock('./agg_add', () => ({
describe('DefaultEditorAgg component', () => {
let defaultProps: DefaultEditorAggGroupProps;
let aggs: IAggConfigs;
let schemas: ISchemas;
let setTouched: jest.Mock;
let setValidity: jest.Mock;
let reorderAggs: jest.Mock;
Expand All @@ -55,6 +56,18 @@ describe('DefaultEditorAgg component', () => {
setTouched = jest.fn();
setValidity = jest.fn();
reorderAggs = jest.fn();
schemas = new Schemas([
{
name: 'metrics',
group: 'metrics',
max: 1,
},
{
name: 'buckets',
group: 'buckets',
max: 1,
},
]);

aggs = {
aggs: [
Expand Down Expand Up @@ -95,18 +108,7 @@ describe('DefaultEditorAgg component', () => {
state: {
data: { aggs },
} as EditorVisState,
schemas: [
{
name: 'metrics',
group: 'metrics',
max: 1,
} as Schema,
{
name: 'buckets',
group: 'buckets',
max: 1,
} as Schema,
],
schemas: schemas.metrics,
setTouched,
setValidity,
reorderAggs,
Expand All @@ -133,6 +135,7 @@ describe('DefaultEditorAgg component', () => {

it('should last bucket has truthy isLastBucket prop', () => {
defaultProps.groupName = 'buckets';
defaultProps.schemas = schemas.buckets;
const comp = mount(<DefaultEditorAggGroup {...defaultProps} />);
const lastAgg = comp.find(DefaultEditorAgg).last();

Expand All @@ -154,6 +157,8 @@ describe('DefaultEditorAgg component', () => {

it('should show add button when schemas count is less than max', () => {
defaultProps.groupName = 'buckets';
defaultProps.schemas = schemas.buckets;
defaultProps.schemas[0].max = 2;
const comp = shallow(<DefaultEditorAggGroup {...defaultProps} />);

expect(comp.find(DefaultEditorAggAdd).exists()).toBeTruthy();
Expand Down
4 changes: 2 additions & 2 deletions src/legacy/core_plugins/vis_default_editor/public/components/agg_group.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ import {
getEnabledMetricAggsCount,
} from './agg_group_helper';
import { aggGroupReducer, initAggsState, AGGS_ACTION_KEYS } from './agg_group_state';
import { Schema, getSchemasByGroup } from '../schemas';
import { Schema } from '../schemas';
import { TimeRange } from '../../../../../plugins/data/public';

export interface DefaultEditorAggGroupProps extends DefaultEditorAggCommonProps {
Expand Down Expand Up @@ -73,7 +73,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

@maryia-lapata maryia-lapata Mar 27, 2020

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.

const schemaNames = schemas.map(s => s.name);
const group: IAggConfig[] = useMemo(
() =>
state.data.aggs!.aggs.filter(
Expand Down
7 changes: 2 additions & 5 deletions src/legacy/core_plugins/vis_default_editor/public/components/sidebar/sidebar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,7 @@ import { DefaultEditorAggCommonProps } from '../agg_common_props';
import { SidebarTitle } from './sidebar_title';
import { PersistedState } from '../../../../../../plugins/visualizations/public';
import { SavedSearch } from '../../../../../../plugins/discover/public';
import { AggGroupNames } from '../../../../../../plugins/data/public';
import { getSchemasByGroup } from '../../schemas';
import { Schema } from '../../schemas';
import { TimeRange } from '../../../../../../plugins/data/public';

interface DefaultEditorSideBarProps {
Expand Down Expand Up @@ -66,9 +65,7 @@ function DefaultEditorSideBar({
const responseAggs = useMemo(() => (state.data.aggs ? state.data.aggs.getResponseAggs() : []), [
state.data.aggs,
]);
const metricSchemas = getSchemasByGroup(vis.type.schemas.all || [], AggGroupNames.Metrics).map(
s => s.name
);
const metricSchemas = (vis.type.schemas.metrics || []).map((s: Schema) => s.name);
const metricAggs = useMemo(
() => responseAggs.filter(agg => metricSchemas.includes(get(agg, 'schema'))),
[responseAggs, metricSchemas]
Expand Down
30 changes: 8 additions & 22 deletions src/legacy/core_plugins/vis_default_editor/public/schemas.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,10 @@
* under the License.
*/

import _ from 'lodash';
import _, { defaults } from 'lodash';

import { Optional } from '@kbn/utility-types';

import { IndexedArray } from 'ui/indexed_array';
import { AggGroupNames, AggParam, IAggGroupNames } from '../../../../plugins/data/public';

export interface ISchemas {
Expand All @@ -45,9 +44,10 @@ export interface Schema {
aggSettings?: any;
}

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

@sulemanof sulemanof Mar 31, 2020

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

@maryia-lapata maryia-lapata Apr 1, 2020

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

@flash1293 flash1293 Apr 1, 2020

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

@lukeelmers lukeelmers Apr 2, 2020

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

@flash1293 flash1293 Apr 2, 2020

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

@lukeelmers lukeelmers Apr 2, 2020

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

@stacey-gammon stacey-gammon Apr 2, 2020

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

[AggGroupNames.Buckets]: Schema[] = [];
[AggGroupNames.Metrics]: Schema[] = [];

constructor(
schemas: Array<
Expand All @@ -70,7 +70,7 @@ export class Schemas {
] as AggParam[];
}

_.defaults(schema, {
defaults(schema, {
min: 0,
max: Infinity,
group: AggGroupNames.Buckets,
Expand All @@ -83,22 +83,12 @@ export class Schemas {
return schema as Schema;
})
.tap((fullSchemas: Schema[]) => {
this.all = new IndexedArray({
index: ['name'],
group: ['group'],
immutable: true,
initialSet: fullSchemas,
});
this.all = fullSchemas;
})
.groupBy('group')
.forOwn((group, groupName) => {
// @ts-ignore
this[groupName] = new IndexedArray({
index: ['name'],
immutable: true,
// @ts-ignore
initialSet: group,
});
this[groupName] = group;
})
.commit();
}
Expand All @@ -107,7 +97,3 @@ export class Schemas {
export const getSchemaByName = (schemas: Schema[], schemaName?: string) => {
return schemas.find(s => s.name === schemaName) || ({} as Schema);
};

export const getSchemasByGroup = (schemas: Schema[], schemaGroup?: string) => {
return schemas.filter(s => s.group === schemaGroup);
};