From 6d6f304d68bbab0859b54d58dfa846a9e85967e8 Mon Sep 17 00:00:00 2001 From: Joe Reuter Date: Tue, 7 May 2019 11:57:37 +0200 Subject: [PATCH 01/10] add nativerenderer component --- x-pack/plugins/lens/public/app_plugin/app.tsx | 9 +- .../utils/native_renderer/__mocks__/react.ts | 12 +++ .../native_renderer/native_renderer.test.tsx | 100 ++++++++++++++++++ .../utils/native_renderer/native_renderer.tsx | 65 ++++++++++++ 4 files changed, 179 insertions(+), 7 deletions(-) create mode 100644 x-pack/plugins/lens/public/utils/native_renderer/__mocks__/react.ts create mode 100644 x-pack/plugins/lens/public/utils/native_renderer/native_renderer.test.tsx create mode 100644 x-pack/plugins/lens/public/utils/native_renderer/native_renderer.tsx diff --git a/x-pack/plugins/lens/public/app_plugin/app.tsx b/x-pack/plugins/lens/public/app_plugin/app.tsx index 3e16a78083092..819109ea6d276 100644 --- a/x-pack/plugins/lens/public/app_plugin/app.tsx +++ b/x-pack/plugins/lens/public/app_plugin/app.tsx @@ -8,20 +8,15 @@ import { I18nProvider } from '@kbn/i18n/react'; import React, { useCallback } from 'react'; import { EditorFrameSetup } from '../types'; +import { NativeRenderer } from '../utils/native_renderer/native_renderer'; export function App({ editorFrame }: { editorFrame: EditorFrameSetup }) { - const renderFrame = useCallback(node => { - if (node !== null) { - editorFrame.render(node); - } - }, []); - return (

Lens

-
+
); diff --git a/x-pack/plugins/lens/public/utils/native_renderer/__mocks__/react.ts b/x-pack/plugins/lens/public/utils/native_renderer/__mocks__/react.ts new file mode 100644 index 0000000000000..b09289b0516ce --- /dev/null +++ b/x-pack/plugins/lens/public/utils/native_renderer/__mocks__/react.ts @@ -0,0 +1,12 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import React from 'react'; + +// This mock overwrites react to use the layout effect hook instead of the normal one. +// This is done to have effects executed synchronously to be able to test them without +// setTimeout hacks. +export default { ...React, useEffect: React.useLayoutEffect }; diff --git a/x-pack/plugins/lens/public/utils/native_renderer/native_renderer.test.tsx b/x-pack/plugins/lens/public/utils/native_renderer/native_renderer.test.tsx new file mode 100644 index 0000000000000..1340a477d570c --- /dev/null +++ b/x-pack/plugins/lens/public/utils/native_renderer/native_renderer.test.tsx @@ -0,0 +1,100 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import React from 'react'; +import { render } from 'react-dom'; +import { NativeRenderer } from './native_renderer'; + +describe('agnostic_renderer', () => { + let mountpoint: Element; + + beforeEach(() => { + mountpoint = document.createElement('div'); + }); + + afterEach(() => { + mountpoint.remove(); + }); + + it('should render element in container', () => { + const renderSpy = jest.fn(); + const testProps = { a: 'abc' }; + + render(, mountpoint); + const containerElement = mountpoint.firstElementChild; + expect(renderSpy).toHaveBeenCalledWith(containerElement, testProps); + }); + + it('should not render again if props do not change', () => { + const renderSpy = jest.fn(); + const testProps = { a: 'abc' }; + + render(, mountpoint); + render(, mountpoint); + expect(renderSpy).toHaveBeenCalledTimes(1); + }); + + it('should not render again if props do not change shallowly', () => { + const renderSpy = jest.fn(); + const testProps = { a: 'abc' }; + + render(, mountpoint); + render(, mountpoint); + expect(renderSpy).toHaveBeenCalledTimes(1); + }); + + it('should render again once if props change', () => { + const renderSpy = jest.fn(); + const testProps = { a: 'abc' }; + + render(, mountpoint); + render(, mountpoint); + render(, mountpoint); + expect(renderSpy).toHaveBeenCalledTimes(2); + const containerElement = mountpoint.firstElementChild; + expect(renderSpy).lastCalledWith(containerElement, { a: 'def' }); + }); + + it('should render again if props are extended', () => { + const renderSpy = jest.fn(); + const testProps = { a: 'abc' }; + + render(, mountpoint); + render(, mountpoint); + expect(renderSpy).toHaveBeenCalledTimes(2); + const containerElement = mountpoint.firstElementChild; + expect(renderSpy).lastCalledWith(containerElement, { a: 'abc', b: 'def' }); + }); + + it('should render again if props are limited', () => { + const renderSpy = jest.fn(); + const testProps = { a: 'abc', b: 'def' }; + + render(, mountpoint); + render(, mountpoint); + expect(renderSpy).toHaveBeenCalledTimes(2); + const containerElement = mountpoint.firstElementChild; + expect(renderSpy).lastCalledWith(containerElement, { a: 'abc' }); + }); + + it('should render a div as container', () => { + const renderSpy = jest.fn(); + const testProps = { a: 'abc' }; + + render(, mountpoint); + const containerElement: Element = mountpoint.firstElementChild!; + expect(containerElement.nodeName).toBe('DIV'); + }); + + it('should render a specified element as container', () => { + const renderSpy = jest.fn(); + const testProps = { a: 'abc' }; + + render(, mountpoint); + const containerElement: Element = mountpoint.firstElementChild!; + expect(containerElement.nodeName).toBe('SPAN'); + }); +}); diff --git a/x-pack/plugins/lens/public/utils/native_renderer/native_renderer.tsx b/x-pack/plugins/lens/public/utils/native_renderer/native_renderer.tsx new file mode 100644 index 0000000000000..46b0f78b0539a --- /dev/null +++ b/x-pack/plugins/lens/public/utils/native_renderer/native_renderer.tsx @@ -0,0 +1,65 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import PropTypes from 'prop-types'; +import React, { useEffect, useRef } from 'react'; + +export interface NativeRendererProps { + render: (domElement: Element, props: T) => void; + actualProps: T; + tag?: string; + children?: never; +} + +function isShallowDifferent(a: T, b: T): boolean { + for (const key in a) { + if (!(key in b) || a[key] !== b[key]) { + return true; + } + } + for (const key in b) { + if (!(key in a) || a[key] !== b[key]) { + return true; + } + } + + return false; +} + +export function NativeRenderer({ render, actualProps, tag }: NativeRendererProps) { + const elementRef = useRef(null); + const propsRef = useRef(null); + + function renderAndUpdate(element: Element) { + elementRef.current = element; + propsRef.current = actualProps; + render(element, actualProps); + } + + useEffect( + () => { + if (elementRef.current && isShallowDifferent(propsRef.current, actualProps)) { + renderAndUpdate(elementRef.current); + } + }, + [actualProps] + ); + + return React.createElement(tag || 'div', { + ref: element => { + if (element && element !== elementRef.current) { + renderAndUpdate(element); + } + }, + }); +} + +NativeRenderer.propTypes = { + render: PropTypes.func.isRequired, + actualProps: PropTypes.object, + + tag: PropTypes.string +}; \ No newline at end of file From 44894dfb69d79a99d4fef5c66865957ca096e891 Mon Sep 17 00:00:00 2001 From: Joe Reuter Date: Tue, 7 May 2019 12:42:27 +0200 Subject: [PATCH 02/10] use native renderer in app and editor frame --- x-pack/plugins/lens/public/app_plugin/app.tsx | 2 +- .../public/editor_frame_plugin/editor_frame.tsx | 14 ++++++-------- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/x-pack/plugins/lens/public/app_plugin/app.tsx b/x-pack/plugins/lens/public/app_plugin/app.tsx index 819109ea6d276..d48badab71016 100644 --- a/x-pack/plugins/lens/public/app_plugin/app.tsx +++ b/x-pack/plugins/lens/public/app_plugin/app.tsx @@ -5,7 +5,7 @@ */ import { I18nProvider } from '@kbn/i18n/react'; -import React, { useCallback } from 'react'; +import React from 'react'; import { EditorFrameSetup } from '../types'; import { NativeRenderer } from '../utils/native_renderer/native_renderer'; diff --git a/x-pack/plugins/lens/public/editor_frame_plugin/editor_frame.tsx b/x-pack/plugins/lens/public/editor_frame_plugin/editor_frame.tsx index 091862e635d99..6dcc14542e21c 100644 --- a/x-pack/plugins/lens/public/editor_frame_plugin/editor_frame.tsx +++ b/x-pack/plugins/lens/public/editor_frame_plugin/editor_frame.tsx @@ -6,6 +6,7 @@ import React from 'react'; import { Datasource, Visualization } from '../types'; +import { NativeRenderer } from '../utils/native_renderer/native_renderer'; interface EditorFrameProps { datasources: { [key: string]: Datasource }; @@ -20,15 +21,12 @@ export function EditorFrame(props: EditorFrameProps) {

Editor Frame

{keys.map(key => ( -
{ - if (domElement) { - props.datasources[key].renderDataPanel(domElement, { - state: {}, - setState: () => {}, - }); - } + render={props.datasources[key].renderDataPanel} + actualProps={{ + state: {}, + setState: () => {}, }} /> ))} From 943fca82e2cc9132199a1d83c7c13c1860c0a537 Mon Sep 17 00:00:00 2001 From: Joe Reuter Date: Tue, 7 May 2019 13:44:40 +0200 Subject: [PATCH 03/10] remove prop types --- .../utils/native_renderer/native_renderer.tsx | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/x-pack/plugins/lens/public/utils/native_renderer/native_renderer.tsx b/x-pack/plugins/lens/public/utils/native_renderer/native_renderer.tsx index 46b0f78b0539a..936433e4b2c78 100644 --- a/x-pack/plugins/lens/public/utils/native_renderer/native_renderer.tsx +++ b/x-pack/plugins/lens/public/utils/native_renderer/native_renderer.tsx @@ -4,7 +4,6 @@ * you may not use this file except in compliance with the Elastic License. */ -import PropTypes from 'prop-types'; import React, { useEffect, useRef } from 'react'; export interface NativeRendererProps { @@ -34,9 +33,9 @@ export function NativeRenderer({ render, actualProps, tag }: NativeRendererPr const propsRef = useRef(null); function renderAndUpdate(element: Element) { - elementRef.current = element; - propsRef.current = actualProps; - render(element, actualProps); + elementRef.current = element; + propsRef.current = actualProps; + render(element, actualProps); } useEffect( @@ -56,10 +55,3 @@ export function NativeRenderer({ render, actualProps, tag }: NativeRendererPr }, }); } - -NativeRenderer.propTypes = { - render: PropTypes.func.isRequired, - actualProps: PropTypes.object, - - tag: PropTypes.string -}; \ No newline at end of file From 731e2647996ba5059b2928711a4adedfe58527a6 Mon Sep 17 00:00:00 2001 From: Joe Reuter Date: Wed, 8 May 2019 08:01:38 +0200 Subject: [PATCH 04/10] add unit test and fix linting issues --- x-pack/plugins/lens/public/index.ts | 2 +- .../utils/native_renderer/__mocks__/react.ts | 2 ++ .../native_renderer/native_renderer.test.tsx | 22 +++++++++++++++++++ 3 files changed, 25 insertions(+), 1 deletion(-) diff --git a/x-pack/plugins/lens/public/index.ts b/x-pack/plugins/lens/public/index.ts index 874249ff75d85..f8f074cdb99bf 100644 --- a/x-pack/plugins/lens/public/index.ts +++ b/x-pack/plugins/lens/public/index.ts @@ -6,8 +6,8 @@ export * from './types'; -import { IScope } from 'angular'; import { render, unmountComponentAtNode } from 'react-dom'; +import { IScope } from 'angular'; import chrome from 'ui/chrome'; import { appSetup, appStop } from './app_plugin'; diff --git a/x-pack/plugins/lens/public/utils/native_renderer/__mocks__/react.ts b/x-pack/plugins/lens/public/utils/native_renderer/__mocks__/react.ts index b09289b0516ce..6039f0888f596 100644 --- a/x-pack/plugins/lens/public/utils/native_renderer/__mocks__/react.ts +++ b/x-pack/plugins/lens/public/utils/native_renderer/__mocks__/react.ts @@ -9,4 +9,6 @@ import React from 'react'; // This mock overwrites react to use the layout effect hook instead of the normal one. // This is done to have effects executed synchronously to be able to test them without // setTimeout hacks. + +// eslint-disable-next-line import/no-default-export export default { ...React, useEffect: React.useLayoutEffect }; diff --git a/x-pack/plugins/lens/public/utils/native_renderer/native_renderer.test.tsx b/x-pack/plugins/lens/public/utils/native_renderer/native_renderer.test.tsx index 1340a477d570c..9ff986a4707aa 100644 --- a/x-pack/plugins/lens/public/utils/native_renderer/native_renderer.test.tsx +++ b/x-pack/plugins/lens/public/utils/native_renderer/native_renderer.test.tsx @@ -46,6 +46,28 @@ describe('agnostic_renderer', () => { expect(renderSpy).toHaveBeenCalledTimes(1); }); + it('should not render again for unchanged callback functions', () => { + const renderSpy = jest.fn(); + const testCallback = () => {}; + const testState = { a: 'abc' }; + + render( + , + mountpoint + ); + render( + , + mountpoint + ); + expect(renderSpy).toHaveBeenCalledTimes(1); + }); + it('should render again once if props change', () => { const renderSpy = jest.fn(); const testProps = { a: 'abc' }; From cc1a0dbaeac72b93140e33aa66fc73943e72256a Mon Sep 17 00:00:00 2001 From: Joe Reuter Date: Wed, 8 May 2019 15:17:26 +0200 Subject: [PATCH 05/10] rename test suite --- .../lens/public/utils/native_renderer/native_renderer.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugins/lens/public/utils/native_renderer/native_renderer.test.tsx b/x-pack/plugins/lens/public/utils/native_renderer/native_renderer.test.tsx index 9ff986a4707aa..3af468e391668 100644 --- a/x-pack/plugins/lens/public/utils/native_renderer/native_renderer.test.tsx +++ b/x-pack/plugins/lens/public/utils/native_renderer/native_renderer.test.tsx @@ -8,7 +8,7 @@ import React from 'react'; import { render } from 'react-dom'; import { NativeRenderer } from './native_renderer'; -describe('agnostic_renderer', () => { +describe('native_renderer', () => { let mountpoint: Element; beforeEach(() => { From 18f9d838e5a358acad1ca2baac2a5681b1582ba7 Mon Sep 17 00:00:00 2001 From: Joe Reuter Date: Thu, 9 May 2019 17:01:04 +0200 Subject: [PATCH 06/10] rename prop and adjust shallow comparison function --- x-pack/plugins/lens/public/app_plugin/app.tsx | 2 +- .../editor_frame_plugin/editor_frame.tsx | 2 +- .../native_renderer/native_renderer.test.tsx | 32 +++++++++---------- .../utils/native_renderer/native_renderer.tsx | 25 ++++++++------- 4 files changed, 32 insertions(+), 29 deletions(-) diff --git a/x-pack/plugins/lens/public/app_plugin/app.tsx b/x-pack/plugins/lens/public/app_plugin/app.tsx index d48badab71016..ba426fa9685a9 100644 --- a/x-pack/plugins/lens/public/app_plugin/app.tsx +++ b/x-pack/plugins/lens/public/app_plugin/app.tsx @@ -16,7 +16,7 @@ export function App({ editorFrame }: { editorFrame: EditorFrameSetup }) {

Lens

- +
); diff --git a/x-pack/plugins/lens/public/editor_frame_plugin/editor_frame.tsx b/x-pack/plugins/lens/public/editor_frame_plugin/editor_frame.tsx index 6dcc14542e21c..bbfd2e6933e3c 100644 --- a/x-pack/plugins/lens/public/editor_frame_plugin/editor_frame.tsx +++ b/x-pack/plugins/lens/public/editor_frame_plugin/editor_frame.tsx @@ -24,7 +24,7 @@ export function EditorFrame(props: EditorFrameProps) { {}, }} diff --git a/x-pack/plugins/lens/public/utils/native_renderer/native_renderer.test.tsx b/x-pack/plugins/lens/public/utils/native_renderer/native_renderer.test.tsx index 3af468e391668..22c0a8d394043 100644 --- a/x-pack/plugins/lens/public/utils/native_renderer/native_renderer.test.tsx +++ b/x-pack/plugins/lens/public/utils/native_renderer/native_renderer.test.tsx @@ -23,7 +23,7 @@ describe('native_renderer', () => { const renderSpy = jest.fn(); const testProps = { a: 'abc' }; - render(, mountpoint); + render(, mountpoint); const containerElement = mountpoint.firstElementChild; expect(renderSpy).toHaveBeenCalledWith(containerElement, testProps); }); @@ -32,8 +32,8 @@ describe('native_renderer', () => { const renderSpy = jest.fn(); const testProps = { a: 'abc' }; - render(, mountpoint); - render(, mountpoint); + render(, mountpoint); + render(, mountpoint); expect(renderSpy).toHaveBeenCalledTimes(1); }); @@ -41,8 +41,8 @@ describe('native_renderer', () => { const renderSpy = jest.fn(); const testProps = { a: 'abc' }; - render(, mountpoint); - render(, mountpoint); + render(, mountpoint); + render(, mountpoint); expect(renderSpy).toHaveBeenCalledTimes(1); }); @@ -54,14 +54,14 @@ describe('native_renderer', () => { render( , mountpoint ); render( , mountpoint ); @@ -72,9 +72,9 @@ describe('native_renderer', () => { const renderSpy = jest.fn(); const testProps = { a: 'abc' }; - render(, mountpoint); - render(, mountpoint); - render(, mountpoint); + render(, mountpoint); + render(, mountpoint); + render(, mountpoint); expect(renderSpy).toHaveBeenCalledTimes(2); const containerElement = mountpoint.firstElementChild; expect(renderSpy).lastCalledWith(containerElement, { a: 'def' }); @@ -84,8 +84,8 @@ describe('native_renderer', () => { const renderSpy = jest.fn(); const testProps = { a: 'abc' }; - render(, mountpoint); - render(, mountpoint); + render(, mountpoint); + render(, mountpoint); expect(renderSpy).toHaveBeenCalledTimes(2); const containerElement = mountpoint.firstElementChild; expect(renderSpy).lastCalledWith(containerElement, { a: 'abc', b: 'def' }); @@ -95,8 +95,8 @@ describe('native_renderer', () => { const renderSpy = jest.fn(); const testProps = { a: 'abc', b: 'def' }; - render(, mountpoint); - render(, mountpoint); + render(, mountpoint); + render(, mountpoint); expect(renderSpy).toHaveBeenCalledTimes(2); const containerElement = mountpoint.firstElementChild; expect(renderSpy).lastCalledWith(containerElement, { a: 'abc' }); @@ -106,7 +106,7 @@ describe('native_renderer', () => { const renderSpy = jest.fn(); const testProps = { a: 'abc' }; - render(, mountpoint); + render(, mountpoint); const containerElement: Element = mountpoint.firstElementChild!; expect(containerElement.nodeName).toBe('DIV'); }); @@ -115,7 +115,7 @@ describe('native_renderer', () => { const renderSpy = jest.fn(); const testProps = { a: 'abc' }; - render(, mountpoint); + render(, mountpoint); const containerElement: Element = mountpoint.firstElementChild!; expect(containerElement.nodeName).toBe('SPAN'); }); diff --git a/x-pack/plugins/lens/public/utils/native_renderer/native_renderer.tsx b/x-pack/plugins/lens/public/utils/native_renderer/native_renderer.tsx index 936433e4b2c78..c1af413c8ed29 100644 --- a/x-pack/plugins/lens/public/utils/native_renderer/native_renderer.tsx +++ b/x-pack/plugins/lens/public/utils/native_renderer/native_renderer.tsx @@ -8,43 +8,46 @@ import React, { useEffect, useRef } from 'react'; export interface NativeRendererProps { render: (domElement: Element, props: T) => void; - actualProps: T; + nativeProps: T; tag?: string; children?: never; } function isShallowDifferent(a: T, b: T): boolean { + if (a === b) { + return false; + } + + if (Object.keys(a).length !== Object.keys(b).length) { + return true; + } + for (const key in a) { if (!(key in b) || a[key] !== b[key]) { return true; } } - for (const key in b) { - if (!(key in a) || a[key] !== b[key]) { - return true; - } - } return false; } -export function NativeRenderer({ render, actualProps, tag }: NativeRendererProps) { +export function NativeRenderer({ render, nativeProps, tag }: NativeRendererProps) { const elementRef = useRef(null); const propsRef = useRef(null); function renderAndUpdate(element: Element) { elementRef.current = element; - propsRef.current = actualProps; - render(element, actualProps); + propsRef.current = nativeProps; + render(element, nativeProps); } useEffect( () => { - if (elementRef.current && isShallowDifferent(propsRef.current, actualProps)) { + if (elementRef.current && isShallowDifferent(propsRef.current, nativeProps)) { renderAndUpdate(elementRef.current); } }, - [actualProps] + [nativeProps] ); return React.createElement(tag || 'div', { From 60cd8911a98b7be1a7b47b7a454b0a9201747c0c Mon Sep 17 00:00:00 2001 From: Joe Reuter Date: Thu, 9 May 2019 17:17:23 +0200 Subject: [PATCH 07/10] add documentation --- .../public/utils/native_renderer/native_renderer.tsx | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/x-pack/plugins/lens/public/utils/native_renderer/native_renderer.tsx b/x-pack/plugins/lens/public/utils/native_renderer/native_renderer.tsx index c1af413c8ed29..f2693ae23c697 100644 --- a/x-pack/plugins/lens/public/utils/native_renderer/native_renderer.tsx +++ b/x-pack/plugins/lens/public/utils/native_renderer/native_renderer.tsx @@ -31,6 +31,15 @@ function isShallowDifferent(a: T, b: T): boolean { return false; } +/** + * A component which takes care of providing a mountpoint for a generic render + * function which takes an html element and an optional props object. + * It also takes care of calling render again if the props object changes. + * By default the mountpoint element will be a div, this can be changed with the + * `tag` prop. + * + * @param props + */ export function NativeRenderer({ render, nativeProps, tag }: NativeRendererProps) { const elementRef = useRef(null); const propsRef = useRef(null); From 0eb1f317c8247ef23d473a2fceda8668b0a2b2eb Mon Sep 17 00:00:00 2001 From: Joe Reuter Date: Thu, 9 May 2019 17:23:25 +0200 Subject: [PATCH 08/10] move native renderer to top level directory --- x-pack/plugins/lens/public/app_plugin/app.tsx | 2 +- .../lens/public/editor_frame_plugin/editor_frame.tsx | 2 +- .../public/{utils => }/native_renderer/__mocks__/react.ts | 0 x-pack/plugins/lens/public/native_renderer/index.ts | 7 +++++++ .../{utils => }/native_renderer/native_renderer.test.tsx | 0 .../public/{utils => }/native_renderer/native_renderer.tsx | 0 6 files changed, 9 insertions(+), 2 deletions(-) rename x-pack/plugins/lens/public/{utils => }/native_renderer/__mocks__/react.ts (100%) create mode 100644 x-pack/plugins/lens/public/native_renderer/index.ts rename x-pack/plugins/lens/public/{utils => }/native_renderer/native_renderer.test.tsx (100%) rename x-pack/plugins/lens/public/{utils => }/native_renderer/native_renderer.tsx (100%) diff --git a/x-pack/plugins/lens/public/app_plugin/app.tsx b/x-pack/plugins/lens/public/app_plugin/app.tsx index ba426fa9685a9..29f1c2eec03f0 100644 --- a/x-pack/plugins/lens/public/app_plugin/app.tsx +++ b/x-pack/plugins/lens/public/app_plugin/app.tsx @@ -8,7 +8,7 @@ import { I18nProvider } from '@kbn/i18n/react'; import React from 'react'; import { EditorFrameSetup } from '../types'; -import { NativeRenderer } from '../utils/native_renderer/native_renderer'; +import { NativeRenderer } from '../native_renderer'; export function App({ editorFrame }: { editorFrame: EditorFrameSetup }) { return ( diff --git a/x-pack/plugins/lens/public/editor_frame_plugin/editor_frame.tsx b/x-pack/plugins/lens/public/editor_frame_plugin/editor_frame.tsx index bbfd2e6933e3c..c74a107b96313 100644 --- a/x-pack/plugins/lens/public/editor_frame_plugin/editor_frame.tsx +++ b/x-pack/plugins/lens/public/editor_frame_plugin/editor_frame.tsx @@ -6,7 +6,7 @@ import React from 'react'; import { Datasource, Visualization } from '../types'; -import { NativeRenderer } from '../utils/native_renderer/native_renderer'; +import { NativeRenderer } from '../native_renderer'; interface EditorFrameProps { datasources: { [key: string]: Datasource }; diff --git a/x-pack/plugins/lens/public/utils/native_renderer/__mocks__/react.ts b/x-pack/plugins/lens/public/native_renderer/__mocks__/react.ts similarity index 100% rename from x-pack/plugins/lens/public/utils/native_renderer/__mocks__/react.ts rename to x-pack/plugins/lens/public/native_renderer/__mocks__/react.ts diff --git a/x-pack/plugins/lens/public/native_renderer/index.ts b/x-pack/plugins/lens/public/native_renderer/index.ts new file mode 100644 index 0000000000000..076e91d950118 --- /dev/null +++ b/x-pack/plugins/lens/public/native_renderer/index.ts @@ -0,0 +1,7 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + + export * from './native_renderer'; \ No newline at end of file diff --git a/x-pack/plugins/lens/public/utils/native_renderer/native_renderer.test.tsx b/x-pack/plugins/lens/public/native_renderer/native_renderer.test.tsx similarity index 100% rename from x-pack/plugins/lens/public/utils/native_renderer/native_renderer.test.tsx rename to x-pack/plugins/lens/public/native_renderer/native_renderer.test.tsx diff --git a/x-pack/plugins/lens/public/utils/native_renderer/native_renderer.tsx b/x-pack/plugins/lens/public/native_renderer/native_renderer.tsx similarity index 100% rename from x-pack/plugins/lens/public/utils/native_renderer/native_renderer.tsx rename to x-pack/plugins/lens/public/native_renderer/native_renderer.tsx From a21b0eaadb129690375afadbd78528f9d4e3e68c Mon Sep 17 00:00:00 2001 From: Joe Reuter Date: Fri, 10 May 2019 10:02:11 +0200 Subject: [PATCH 09/10] add test, fix linting error and improve shallow equal comparison --- .../lens/public/native_renderer/index.ts | 2 +- .../native_renderer/native_renderer.test.tsx | 12 +++++++++++ .../native_renderer/native_renderer.tsx | 21 ++++++++++++++----- 3 files changed, 29 insertions(+), 6 deletions(-) diff --git a/x-pack/plugins/lens/public/native_renderer/index.ts b/x-pack/plugins/lens/public/native_renderer/index.ts index 076e91d950118..0ef9bd8807bc5 100644 --- a/x-pack/plugins/lens/public/native_renderer/index.ts +++ b/x-pack/plugins/lens/public/native_renderer/index.ts @@ -4,4 +4,4 @@ * you may not use this file except in compliance with the Elastic License. */ - export * from './native_renderer'; \ No newline at end of file +export * from './native_renderer'; diff --git a/x-pack/plugins/lens/public/native_renderer/native_renderer.test.tsx b/x-pack/plugins/lens/public/native_renderer/native_renderer.test.tsx index 22c0a8d394043..089b1ed02d166 100644 --- a/x-pack/plugins/lens/public/native_renderer/native_renderer.test.tsx +++ b/x-pack/plugins/lens/public/native_renderer/native_renderer.test.tsx @@ -80,6 +80,18 @@ describe('native_renderer', () => { expect(renderSpy).lastCalledWith(containerElement, { a: 'def' }); }); + it('should render again once if props is just a string', () => { + const renderSpy = jest.fn(); + const testProps = 'abc'; + + render(, mountpoint); + render(, mountpoint); + render(, mountpoint); + expect(renderSpy).toHaveBeenCalledTimes(2); + const containerElement = mountpoint.firstElementChild; + expect(renderSpy).lastCalledWith(containerElement, 'def'); + }); + it('should render again if props are extended', () => { const renderSpy = jest.fn(); const testProps = { a: 'abc' }; diff --git a/x-pack/plugins/lens/public/native_renderer/native_renderer.tsx b/x-pack/plugins/lens/public/native_renderer/native_renderer.tsx index f2693ae23c697..f0eb4b829c153 100644 --- a/x-pack/plugins/lens/public/native_renderer/native_renderer.tsx +++ b/x-pack/plugins/lens/public/native_renderer/native_renderer.tsx @@ -13,17 +13,28 @@ export interface NativeRendererProps { children?: never; } -function isShallowDifferent(a: T, b: T): boolean { - if (a === b) { +function is(x: unknown, y: unknown) { + return (x === y && (x !== 0 || 1 / (x as number) === 1 / (y as number))) || (x !== x && y !== y); +} + +function isShallowDifferent(objA: T, objB: T): boolean { + if (is(objA, objB)) { return false; } - if (Object.keys(a).length !== Object.keys(b).length) { + if (typeof objA !== 'object' || objA === null || typeof objB !== 'object' || objB === null) { + return true; + } + + const keysA = Object.keys(objA) as Array; + const keysB = Object.keys(objB) as Array; + + if (keysA.length !== keysB.length) { return true; } - for (const key in a) { - if (!(key in b) || a[key] !== b[key]) { + for (let i = 0; i < keysA.length; i++) { + if (!window.hasOwnProperty.call(objB, keysA[i]) || !is(objA[keysA[i]], objB[keysA[i]])) { return true; } } From 4baf6b196405d6f563e4078fa8b0fbe0df82d83d Mon Sep 17 00:00:00 2001 From: Joe Reuter Date: Fri, 10 May 2019 16:49:44 +0200 Subject: [PATCH 10/10] fix native renderer tests --- .../public/native_renderer/__mocks__/react.ts | 14 --- .../native_renderer/native_renderer.test.tsx | 87 +++++++++++++++---- 2 files changed, 70 insertions(+), 31 deletions(-) delete mode 100644 x-pack/plugins/lens/public/native_renderer/__mocks__/react.ts diff --git a/x-pack/plugins/lens/public/native_renderer/__mocks__/react.ts b/x-pack/plugins/lens/public/native_renderer/__mocks__/react.ts deleted file mode 100644 index 6039f0888f596..0000000000000 --- a/x-pack/plugins/lens/public/native_renderer/__mocks__/react.ts +++ /dev/null @@ -1,14 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License; - * you may not use this file except in compliance with the Elastic License. - */ - -import React from 'react'; - -// This mock overwrites react to use the layout effect hook instead of the normal one. -// This is done to have effects executed synchronously to be able to test them without -// setTimeout hacks. - -// eslint-disable-next-line import/no-default-export -export default { ...React, useEffect: React.useLayoutEffect }; diff --git a/x-pack/plugins/lens/public/native_renderer/native_renderer.test.tsx b/x-pack/plugins/lens/public/native_renderer/native_renderer.test.tsx index 089b1ed02d166..af5196165f9a5 100644 --- a/x-pack/plugins/lens/public/native_renderer/native_renderer.test.tsx +++ b/x-pack/plugins/lens/public/native_renderer/native_renderer.test.tsx @@ -7,6 +7,14 @@ import React from 'react'; import { render } from 'react-dom'; import { NativeRenderer } from './native_renderer'; +import { act } from 'react-dom/test-utils'; + +function renderAndTriggerHooks(element: JSX.Element, mountpoint: Element) { + // act takes care of triggering state hooks + act(() => { + render(element, mountpoint); + }); +} describe('native_renderer', () => { let mountpoint: Element; @@ -23,7 +31,10 @@ describe('native_renderer', () => { const renderSpy = jest.fn(); const testProps = { a: 'abc' }; - render(, mountpoint); + renderAndTriggerHooks( + , + mountpoint + ); const containerElement = mountpoint.firstElementChild; expect(renderSpy).toHaveBeenCalledWith(containerElement, testProps); }); @@ -32,8 +43,14 @@ describe('native_renderer', () => { const renderSpy = jest.fn(); const testProps = { a: 'abc' }; - render(, mountpoint); - render(, mountpoint); + renderAndTriggerHooks( + , + mountpoint + ); + renderAndTriggerHooks( + , + mountpoint + ); expect(renderSpy).toHaveBeenCalledTimes(1); }); @@ -41,8 +58,14 @@ describe('native_renderer', () => { const renderSpy = jest.fn(); const testProps = { a: 'abc' }; - render(, mountpoint); - render(, mountpoint); + renderAndTriggerHooks( + , + mountpoint + ); + renderAndTriggerHooks( + , + mountpoint + ); expect(renderSpy).toHaveBeenCalledTimes(1); }); @@ -72,9 +95,18 @@ describe('native_renderer', () => { const renderSpy = jest.fn(); const testProps = { a: 'abc' }; - render(, mountpoint); - render(, mountpoint); - render(, mountpoint); + renderAndTriggerHooks( + , + mountpoint + ); + renderAndTriggerHooks( + , + mountpoint + ); + renderAndTriggerHooks( + , + mountpoint + ); expect(renderSpy).toHaveBeenCalledTimes(2); const containerElement = mountpoint.firstElementChild; expect(renderSpy).lastCalledWith(containerElement, { a: 'def' }); @@ -84,9 +116,12 @@ describe('native_renderer', () => { const renderSpy = jest.fn(); const testProps = 'abc'; - render(, mountpoint); - render(, mountpoint); - render(, mountpoint); + renderAndTriggerHooks( + , + mountpoint + ); + renderAndTriggerHooks(, mountpoint); + renderAndTriggerHooks(, mountpoint); expect(renderSpy).toHaveBeenCalledTimes(2); const containerElement = mountpoint.firstElementChild; expect(renderSpy).lastCalledWith(containerElement, 'def'); @@ -96,8 +131,14 @@ describe('native_renderer', () => { const renderSpy = jest.fn(); const testProps = { a: 'abc' }; - render(, mountpoint); - render(, mountpoint); + renderAndTriggerHooks( + , + mountpoint + ); + renderAndTriggerHooks( + , + mountpoint + ); expect(renderSpy).toHaveBeenCalledTimes(2); const containerElement = mountpoint.firstElementChild; expect(renderSpy).lastCalledWith(containerElement, { a: 'abc', b: 'def' }); @@ -107,8 +148,14 @@ describe('native_renderer', () => { const renderSpy = jest.fn(); const testProps = { a: 'abc', b: 'def' }; - render(, mountpoint); - render(, mountpoint); + renderAndTriggerHooks( + , + mountpoint + ); + renderAndTriggerHooks( + , + mountpoint + ); expect(renderSpy).toHaveBeenCalledTimes(2); const containerElement = mountpoint.firstElementChild; expect(renderSpy).lastCalledWith(containerElement, { a: 'abc' }); @@ -118,7 +165,10 @@ describe('native_renderer', () => { const renderSpy = jest.fn(); const testProps = { a: 'abc' }; - render(, mountpoint); + renderAndTriggerHooks( + , + mountpoint + ); const containerElement: Element = mountpoint.firstElementChild!; expect(containerElement.nodeName).toBe('DIV'); }); @@ -127,7 +177,10 @@ describe('native_renderer', () => { const renderSpy = jest.fn(); const testProps = { a: 'abc' }; - render(, mountpoint); + renderAndTriggerHooks( + , + mountpoint + ); const containerElement: Element = mountpoint.firstElementChild!; expect(containerElement.nodeName).toBe('SPAN'); });