TypeScriptify visualization components #20940

timroes · 2018-07-18T17:02:43Z

This PR converts the components used in the visualization rendering infrastructure to TypeScript. It also fixes a couple of issues:

We now call onInit also when no data should be shown (earlier onInit was only called in case the no data screen wasn't shown)
We now pass in uiState properly to the <VisualizationChart/> component (which was missing earlier), so it can now again correctly calculate the update status for uiState.

It also improves the code a bit:

Replace the "storing observer mechanism" by using a Subject in VisualizationChart

elasticmachine · 2018-07-18T18:17:18Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

markov00

Code LGTM.
I've left few minor comments and one question/doubt.
Could you please rebase/merge master so we can test this after the flickering fix?

src/ui/public/visualize/components/visualization.test.js

@@ -69,6 +52,28 @@ describe('Visualization', () => {
    set: () => {}


markov00 · 2018-07-19T08:44:55Z

src/ui/public/visualize/components/visualization.tsx

+    if (this.state.showNoResultsMessage && this.props.onInit) {
+      this.props.onInit();
+    }
+  }


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.

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.

markov00 · 2018-07-19T10:10:29Z

src/ui/public/visualize/components/visualization_chart.tsx

+      filter(
+        ({ vis, visData, container }) => vis && container && (!vis.type.requiresSearch || visData)
+      ),
+      debounceTime(100),


I know that this was here before, but is there any more "pragmatic" way to avoid this?
Adding timers and debouncing usually increase the possibility to have flaky tests because we are not tied to a specific and clear state and we are saying: ok maybe there will be other 2 or 3 subsequent call/updates of the renderSubject one next to the other and it's better to wait some time.
I think this kind of uncertainty brings the flakyness to our test.
Any thoughts on that?

The most common use-case at the moment is resizing the visualization, e.g. by resizing the window or resizing the dashboard panel it's in. In case we don't debounce, we are very laggy due to too many renders, while scaling a dashboard panel. Though it wouldn't be needed in all rendering triggering cases, but I am not sure if we actually simplify the logic and increase maintainability if we separate between renders, that need the debounce and some that doesn't.

elasticmachine · 2018-07-19T12:19:41Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

timroes · 2018-07-19T12:29:41Z

Jenkins, test this

elasticmachine · 2018-07-19T13:51:32Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

weltenwort

Not being too familiar with the visualization code, I could not find anything wrong with the TypeScript usage. I left some comments regarding the React usage below. If you think these will be resolved in follow-up PRs anyway, feel free to ignore them for now.

weltenwort · 2018-07-19T11:28:10Z

src/ui/public/vis/vis_types/vis_type.d.ts

+
+export class VisualizationController {
+  constructor(element: HTMLElement, vis: Vis);
+  public render(visData: any, update: { [key in Status]: boolean }): Promise<void>;


Given that { [key in Status]: boolean } is used in more than one place now (e.g. here and the return value of getUpdateStatus), how about factoring it out into its own type in update_status.ts?

export type StatusUpdates<Keys extends Status> = { [key in Keys]: boolean; }

I think that's actually not working that well, since in the getUpdateStatus we actually use a different index signature, since we actually make sure, it returns an object only containing the status updates, that have been request as parameter. So I currently don't see any other place we would be using this - and hopefully it won't become more, because we are trying to get rid of the status calculation and not increasing it :D

good point 👍

weltenwort · 2018-07-19T11:56:41Z

src/ui/public/visualize/components/visualization.tsx

+    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!');


As we discussed directly already, I think abusing this function as a "validator" for props by consciously throwing an error is problematic for a few reasons. Primarily, react components do not support write-once properties which means this violates the general component contract. On the contrary, props that communicate state (as the name of uiState suggests it does) must change identity to signal that state change. Additionally, which stack trace this error bubbles up through is an implementation detail that should not be relied upon.

Instead I would recommend to check for a change of uiState here to possibly set a flag on the state and render an appropriate error message in render(). Changes to listenOnChange could be easily handled by checking for a change in componentDidUpdate and (un)subscribing accordingly.

weltenwort · 2018-07-19T12:00:29Z

src/ui/public/visualize/components/visualization.tsx

+      throw new Error('Changing listenOnChange or uiState props is not allowed!');
+    }
+
+    const showNoResultsMessage = shouldShowNoResultsMessage(props.vis, props.visData);


How about moving this to render()? See "What about memoization?" in "You Probably Don't Need Derived State"

I addressed that and memoized the function instead (way better readable also :D)

weltenwort · 2018-07-19T12:04:48Z

src/ui/public/visualize/components/visualization.tsx

+  }
+
+  public componentWillUnmount() {
+    this.props.uiState.off('change', this.onUiStateChanged);


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.

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?

weltenwort · 2018-07-19T14:13:58Z

src/ui/public/visualize/components/visualization_noresults.tsx

+
+  public componentDidUpdate() {
+    if (this.props.onInit) {
+      this.props.onInit();


Out of curiosity, why should onInit() be called after every render?

If you use the visualization loader to render a visualization, you might end up calling it again, in case something changed on your behalf. If you then render "over" an already mounted component, that component would just update, but nevertheless we want to resolve the promise returned from that called (that's passed to the onInit method of the chart).

elasticmachine · 2018-07-19T20:08:23Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-07-19T21:44:34Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-07-20T09:08:08Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

timroes · 2018-07-20T09:49:18Z

Jenkins, test this

weltenwort

That looks quite good (pending green CI)! Thanks for investing the time.

weltenwort · 2018-07-20T09:20:49Z

src/ui/public/utils/memoize.ts

+ */
+function memoizeLast<T extends (...args: any[]) => any>(func: T): T {
+  let prevResult: any;
+  let called: boolean = false;


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.

elasticmachine · 2018-07-20T11:05:09Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-07-20T11:45:14Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-07-23T09:14:02Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

timroes · 2018-07-23T12:34:19Z

Jenkins, test this

elasticmachine · 2018-07-23T14:16:46Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* Refactor vis components to TypeScript * Fix issue with ResizeChecker * Fix calling onInit for no data * Explicit named export * Add title to vistype * Fix error in test file * Move onInit to no VisualizationNoResults * Make listenOnChange changeable * Add memoize util * Use memoize for no results check * Address issue with uiState * Optimize memoize function

bhavyarm · 2018-08-07T08:03:14Z

How do we test this @timroes ? Do I need to look at anything in UI or it's just code changes? Thanks!

timroes · 2018-08-07T08:07:46Z

@bhavyarm This should not change any functionality but should just be code changes.

Tim Roes added 5 commits July 18, 2018 15:07

Refactor vis components to TypeScript

dff08ed

Fix issue with ResizeChecker

3574c37

Fix calling onInit for no data

ca679e0

Explicit named export

91860ac

Add title to vistype

83b66fd

timroes added bug Fixes for quality problems that affect the customer experience chore Feature:Visualizations Generic visualization features (in case no more specific feature label is available) v7.0.0 v6.4.0 labels Jul 18, 2018

timroes requested review from markov00 and ppisljar July 18, 2018 17:02

markov00 reviewed Jul 19, 2018

View reviewed changes

Tim Roes added 3 commits July 19, 2018 12:46

Merge branch 'master' into ts-visualization

231d214

Fix error in test file

9062384

Move onInit to no VisualizationNoResults

06ee085

markov00 approved these changes Jul 19, 2018

View reviewed changes

weltenwort reviewed Jul 19, 2018

View reviewed changes

Make listenOnChange changeable

361e2a8

Tim Roes added 2 commits July 19, 2018 22:17

Add memoize util

8f06918

Use memoize for no results check

b8e57cc

Address issue with uiState

dc4c456

weltenwort approved these changes Jul 20, 2018

View reviewed changes

Optimize memoize function

a919b5d

timroes mentioned this pull request Jul 20, 2018

TypeScriptify visualize loader #21025

Merged

Merge branch 'master' into ts-visualization

84733cd

timroes merged commit d171aa8 into elastic:master Jul 23, 2018

timroes deleted the ts-visualization branch July 23, 2018 14:21

timroes mentioned this pull request Jul 23, 2018

[6.x] TypeScriptify visualization components (#20940) #21086

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TypeScriptify visualization components #20940

TypeScriptify visualization components #20940

timroes commented Jul 18, 2018 •

edited

Loading

elasticmachine commented Jul 18, 2018

markov00 left a comment •

edited

Loading

This comment was marked as resolved.

markov00 Jul 19, 2018

timroes Jul 19, 2018

markov00 Jul 19, 2018

timroes Jul 19, 2018

elasticmachine commented Jul 19, 2018

timroes commented Jul 19, 2018

elasticmachine commented Jul 19, 2018

weltenwort left a comment

weltenwort Jul 19, 2018

timroes Jul 19, 2018

weltenwort Jul 19, 2018

weltenwort Jul 19, 2018

weltenwort Jul 19, 2018

timroes Jul 19, 2018

weltenwort Jul 19, 2018

timroes Jul 20, 2018 •

edited

Loading

weltenwort Jul 19, 2018

timroes Jul 19, 2018

elasticmachine commented Jul 19, 2018

elasticmachine commented Jul 19, 2018

elasticmachine commented Jul 20, 2018

timroes commented Jul 20, 2018

weltenwort left a comment

weltenwort Jul 20, 2018

elasticmachine commented Jul 20, 2018

elasticmachine commented Jul 20, 2018

elasticmachine commented Jul 23, 2018

timroes commented Jul 23, 2018

elasticmachine commented Jul 23, 2018

bhavyarm commented Aug 7, 2018

timroes commented Aug 7, 2018

		@@ -69,6 +52,28 @@ describe('Visualization', () => {
		set: () => {}

TypeScriptify visualization components #20940

TypeScriptify visualization components #20940

Conversation

timroes commented Jul 18, 2018 • edited Loading

elasticmachine commented Jul 18, 2018

💚 Build Succeeded

markov00 left a comment • edited Loading

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Jul 19, 2018

💔 Build Failed

timroes commented Jul 19, 2018

elasticmachine commented Jul 19, 2018

💚 Build Succeeded

weltenwort left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timroes Jul 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Jul 19, 2018

💚 Build Succeeded

elasticmachine commented Jul 19, 2018

💔 Build Failed

elasticmachine commented Jul 20, 2018

💔 Build Failed

timroes commented Jul 20, 2018

weltenwort left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Jul 20, 2018

💔 Build Failed

elasticmachine commented Jul 20, 2018

💔 Build Failed

elasticmachine commented Jul 23, 2018

💔 Build Failed

timroes commented Jul 23, 2018

elasticmachine commented Jul 23, 2018

💚 Build Succeeded

bhavyarm commented Aug 7, 2018

timroes commented Aug 7, 2018

timroes commented Jul 18, 2018 •

edited

Loading

markov00 left a comment •

edited

Loading

timroes Jul 20, 2018 •

edited

Loading