Skip to content

Commit

Permalink
fix: specs swaps correctly reflected in state (opensearch-project#901)
Browse files Browse the repository at this point in the history
  • Loading branch information
markov00 authored Nov 17, 2020
1 parent e97f4ab commit a94347f
Show file tree
Hide file tree
Showing 3 changed files with 97 additions and 15 deletions.
2 changes: 1 addition & 1 deletion packages/osd-charts/src/state/chart_state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,7 @@ export const chartStoreReducer = (chartId: string) => {
...state,
specsInitialized: false,
chartRendered: false,
specParsing: true,
specParsing: false,
specs: {
...rest,
},
Expand Down
94 changes: 94 additions & 0 deletions packages/osd-charts/src/state/spec_factory.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import { mount, ReactWrapper } from 'enzyme';
import React from 'react';

import { BarSeries } from '../chart_types/xy_chart/specs/bar_series';
import { Chart } from '../components/chart';
import { MockSeriesSpec } from '../mocks/specs/specs';
import { Settings } from '../specs/settings';
import { DebugState } from './types';

function getDebugState(wrapper: ReactWrapper): DebugState {
const statusComponent = wrapper.find('.echChartStatus');
const debugState = statusComponent.getDOMNode().getAttribute('data-ech-debug-state');
const parsedDebugState = JSON.parse(debugState || '');
return parsedDebugState as DebugState;
}

describe('Spec factory', () => {
const spec1 = MockSeriesSpec.bar({ id: 'spec1', data: [{ x: 0, y: 1 }] });
const spec2 = MockSeriesSpec.bar({ id: 'spec2', data: [{ x: 0, y: 2 }] });

it('We can switch specs props between react component', () => {
const wrapper = mount(
<Chart size={[100, 100]} id="chart1">
<Settings debugState />
<BarSeries {...spec1} />;
<BarSeries {...spec2} />;
</Chart>,
);
let debugState = getDebugState(wrapper);
expect(debugState.bars).toHaveLength(2);
wrapper.setProps({
children: (
<>
<Settings debugState />
<BarSeries {...spec2} />;
<BarSeries {...spec1} />;
</>
),
});
debugState = getDebugState(wrapper);
expect(debugState.bars).toHaveLength(2);
});

it('We can switch specs ids between react component', () => {
const wrapper = mount(
<Chart size={[100, 100]} id="chart1">
<Settings debugState />
<BarSeries {...spec1} />;
<BarSeries {...spec2} />;
</Chart>,
);
let debugState = getDebugState(wrapper);
expect(debugState.bars).toHaveLength(2);
wrapper.setProps({
children: (
<>
<Settings debugState />
<BarSeries {...spec1} id={spec2.id} />;
<BarSeries {...spec2} id={spec1.id} />;
</>
),
});
debugState = getDebugState(wrapper);

expect(debugState.bars).toHaveLength(2);

// different id same y
expect(debugState.bars?.[0].name).toBe('spec2');
expect(debugState.bars?.[0].bars[0].y).toBe(1);

// different id same y
expect(debugState.bars?.[1].name).toBe('spec1');
expect(debugState.bars?.[1].bars[0].y).toBe(2);
});
});
16 changes: 2 additions & 14 deletions packages/osd-charts/src/state/spec_factory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
* under the License.
*/

import { useEffect, useRef } from 'react';
import { useEffect } from 'react';
import { connect } from 'react-redux';
import { Dispatch, bindActionCreators } from 'redux';

Expand All @@ -30,26 +30,14 @@ export interface DispatchProps {
removeSpec: (id: string) => void;
}

function usePrevious(value: string) {
const ref = useRef<string>();
useEffect(() => {
ref.current = value;
});
return ref.current;
}

/** @internal */
export function specComponentFactory<U extends Spec, D extends keyof U>(
defaultProps: Pick<U, D | 'chartType' | 'specType'>,
) {
/* eslint-disable no-shadow, react-hooks/exhaustive-deps, unicorn/consistent-function-scoping */
const SpecInstance = (props: U & DispatchProps) => {
const prevId = usePrevious(props.id);
const { removeSpec, upsertSpec, ...SpecInstance } = props;
useEffect(() => {
if (prevId && prevId !== props.id) {
removeSpec(prevId);
}
upsertSpec(SpecInstance);
});
useEffect(
Expand Down Expand Up @@ -80,7 +68,7 @@ export function getConnect() {
* Redux assumes shallowEqual for all connected components
*
* This causes an issue where the specs are cleared and memoized spec components will never be
* rerendered and thus never re-upserted to the state. Setting pure to false solves this issue
* re-rendered and thus never re-upserted to the state. Setting pure to false solves this issue
* and doesn't cause traditional performance degradations.
*/
return connect(null, mapDispatchToProps, null, { pure: false });
Expand Down

0 comments on commit a94347f

Please sign in to comment.