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.

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

TypeScriptify visualization components #20940

Merged
merged 14 commits into from
Jul 23, 2018
50 changes: 50 additions & 0 deletions src/ui/public/utils/memoize.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/*
* 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 { memoizeLast } from './memoize';

describe('memoizeLast', () => {
type SumFn = (a: number, b: number) => number;
let originalSum: SumFn;
let memoizedSum: SumFn;

beforeEach(() => {
originalSum = jest.fn((a, b) => a + b);
memoizedSum = memoizeLast(originalSum);
});

it('should call through function', () => {
expect(memoizedSum(26, 16)).toBe(42);
expect(originalSum).toHaveBeenCalledWith(26, 16);
});

it('should memoize the last call', () => {
memoizedSum(26, 16);
expect(originalSum).toHaveBeenCalledTimes(1);
memoizedSum(26, 16);
expect(originalSum).toHaveBeenCalledTimes(1);
});

it('should use parameters as cache keys', () => {
expect(memoizedSum(26, 16)).toBe(42);
expect(originalSum).toHaveBeenCalledTimes(1);
expect(memoizedSum(16, 26)).toBe(42);
expect(originalSum).toHaveBeenCalledTimes(2);
});
});
51 changes: 51 additions & 0 deletions src/ui/public/utils/memoize.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
/*
* 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.
*/

/**
* A simple memoize function, that only stores the last returned value
* and uses the identity of all passed parameters as a cache key.
*/
function memoizeLast<T extends (...args: any[]) => any>(func: T): T {
let prevResult: any;
let called: boolean = false;
Copy link
Member

Choose a reason for hiding this comment

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

We could get rid of called by initializing prevArgs to an array containing a unique symbol that is not exported from this module. Then the conditional inside memoizedFunction would never be true on the first execution.

let prevThis: any;
let prevArgs: any[];

// We need to use a `function` here for proper this passing.
// tslint:disable-next-line:only-arrow-functions
const memoizedFunction = function(this: any, ...args: any[]) {
if (
called &&
prevThis === this &&
prevArgs.length === args.length &&
args.every((arg, index) => arg === prevArgs[index])
) {
return prevResult;
}
called = true;
prevThis = this;
prevArgs = args;
prevResult = func.apply(this, args);
return prevResult;
} as T;

return memoizedFunction;
}

export { memoizeLast };
55 changes: 26 additions & 29 deletions src/ui/public/visualize/components/visualization.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import { get } from 'lodash';
import React from 'react';

import { PersistedState } from '../../persisted_state';
import { memoizeLast } from '../../utils/memoize';
import { Vis } from '../../vis';
import { VisualizationChart } from './visualization_chart';
import { VisualizationNoResults } from './visualization_noresults';
Expand All @@ -43,51 +44,28 @@ interface VisualizationProps {
visData: any;
}

interface VisualizationState {
listenOnChange: boolean;
showNoResultsMessage: boolean;
}

export class Visualization extends React.Component<VisualizationProps, VisualizationState> {
public static getDerivedStateFromProps(
props: VisualizationProps,
prevState: VisualizationState
): Partial<VisualizationState> | null {
const listenOnChangeChanged = props.listenOnChange !== prevState.listenOnChange;
const uiStateChanged = props.uiState && props.uiState !== props.vis.getUiState();
if (listenOnChangeChanged || uiStateChanged) {
throw new Error('Changing listenOnChange or uiState props is not allowed!');
}

const showNoResultsMessage = shouldShowNoResultsMessage(props.vis, props.visData);
if (prevState.showNoResultsMessage !== showNoResultsMessage) {
return { showNoResultsMessage };
}
return null;
}
export class Visualization extends React.Component<VisualizationProps> {
private showNoResultsMessage = memoizeLast(shouldShowNoResultsMessage);

constructor(props: VisualizationProps) {
super(props);

const { vis, visData, uiState, listenOnChange } = props;
const { vis, uiState, listenOnChange } = props;

vis._setUiState(props.uiState);
if (listenOnChange) {
uiState.on('change', this.onUiStateChanged);
}

this.state = {
listenOnChange,
showNoResultsMessage: shouldShowNoResultsMessage(vis, visData),
};
}

public render() {
const { vis, visData, onInit, uiState } = this.props;

const noResults = this.showNoResultsMessage(vis, visData);

return (
<div className="visualization">
{this.state.showNoResultsMessage ? (
{noResults ? (
<VisualizationNoResults onInit={onInit} />
) : (
<VisualizationChart vis={vis} visData={visData} onInit={onInit} uiState={uiState} />
Expand All @@ -96,10 +74,29 @@ export class Visualization extends React.Component<VisualizationProps, Visualiza
);
}

public shouldComponentUpdate(nextProps: VisualizationProps): boolean {
if (nextProps.uiState !== this.props.uiState) {
throw new Error('Changing uiState on <Visualization/> is not supported!');
}
return true;
}
Copy link
Member

Choose a reason for hiding this comment

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

What about removing this logic from the Visualization component and add it to the VisualizationNoResults component?

You can pass the onInit function to VisualizationNoResults, build it as a Component instead of a SFC and call the onInit there.

This will decouple the onInit calls from the Visualization and moving the initialization callback responsibility to the rendered components. In a way it will be more clear to have the onInit inside each rendered component, than having a statement that check this on it's parent component.

In the future if we want to move the VisualizationNoResults logic/display inside the VisualizationChart we can do just removing the VisualizationNoResults component and passing showNoResultsMessage , without change any other logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Will do that. Also I think we should discuss next week with Peter, if we can actually already remove it, because it really seems unused inside Kibana, and we can just declare that a change for visualizations in general, to bring their own no data screen.


public componentWillUnmount() {
this.props.uiState.off('change', this.onUiStateChanged);
Copy link
Member

Choose a reason for hiding this comment

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

So if the uiState was replaced with a new object and getDerivedStateFromProps threw the problematic error, the component will unmount and this will be called. Is this.props.uiState still the old object that we subscribed on or do we leak memory here?

We should probably either copy uiState to this.state (meh) or, on subscription, save an unsubscribe function:

private unsubscribeFromUiStateChange: (() => void)) = () => undefined;

private subscribeToUiStateChange() {
  const { uiState } = this.props;

  uiState.on('change', this.onUiStateChanged);
  
  this.unsubscribeFromUiStateChange = () => {
    uiState.off('change', this.onUiStateChanged);
    this.unsubscribeFromUiStateChange = () => undefined;
  }
}

public componentWillUnmount() {
  this.unsubscribeFromUiStateChange();
}

We could also just un- and re-subscribe in componentDidUpdate() whenever the prop changed, but as long as getDerivedStateFromProps() throws that error, I would rely on that working quite right.

Copy link
Contributor Author

@timroes timroes Jul 20, 2018

Choose a reason for hiding this comment

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

I addressed this issue now in a different way. Since I was able to remove the state completely, using getDerivedStateFromProps would anyway be nasty, because as long as that method exists I need to initialize an empty state (even if my typing said there is none), see previous test failure.

So I actually moved the check to shouldComponentUpdate and throw the error there. This will also have the sideeffect, that in case we throw there, the properties in componenWillUnmount are still the old ones, and we deregister from the correct uiState. (see that example: http://jsfiddle.net/us5ojhwv/10/)

Of course the ideal solution would still be allowing that uiState switch, but I fear that's a bit too much trouble to change right now, and since I also didn't introduce it with this PR, I would rather postpone this to a later PR/point in time, if it would be okay for you?

}

public componentDidUpdate(prevProps: VisualizationProps) {
const { listenOnChange } = this.props;
// If the listenOnChange prop changed, we need to register or deregister from uiState
if (prevProps.listenOnChange !== listenOnChange) {
if (listenOnChange) {
this.props.uiState.on('change', this.onUiStateChanged);
} else {
this.props.uiState.off('change', this.onUiStateChanged);
}
}
}

/**
* In case something in the uiState changed, we need to force a redraw of
* the visualization, since these changes could effect visualization rendering.
Expand Down