From 92d309a1e280b20ac751dee7c64ffcaeccd7e3c8 Mon Sep 17 00:00:00 2001 From: Javier Campa Lopez Date: Wed, 11 Jul 2018 15:09:05 +0200 Subject: [PATCH 1/7] Added support for SSR on regions and refactored to remove handlers --- packages/frint-react/src/components/Region.js | 171 +++++++++++++++--- .../frint-react/src/components/Region.spec.js | 58 ++++++ 2 files changed, 208 insertions(+), 21 deletions(-) diff --git a/packages/frint-react/src/components/Region.js b/packages/frint-react/src/components/Region.js index 26b445b7..df33606e 100644 --- a/packages/frint-react/src/components/Region.js +++ b/packages/frint-react/src/components/Region.js @@ -1,11 +1,10 @@ /* eslint-disable no-console, no-underscore-dangle, import/no-extraneous-dependencies */ import React from 'react'; import PropTypes from 'prop-types'; +import isEqual from 'lodash/isEqual'; +import zipWith from 'lodash/zipWith'; -import composeHandlers from 'frint-component-utils/lib/composeHandlers'; -import RegionHandler from 'frint-component-handlers/lib/RegionHandler'; - -import ReactHandler from '../handlers/ReactHandler'; +import getMountableComponent from './getMountableComponent'; export default class Region extends React.Component { static propTypes = { @@ -19,35 +18,165 @@ export default class Region extends React.Component { app: PropTypes.object, }; - constructor(...args) { - super(...args); + static sendProps(appInstance, props) { + const regionService = appInstance.get(appInstance.options.providerNames.region); - this._handler = composeHandlers( - ReactHandler, - RegionHandler, - { - component: this, - }, - ); + if (!regionService) { + return; + } - this.state = this._handler.getInitialData(); + regionService.emit(props); + } + + constructor(props, context) { + super(props, context); + + if (context.app) { + const rootApp = context.app.getRootApp(); + const list = rootApp._appsCollection.filter(({ regions }) => { + return regions.some(name => props.name === name); + }); + + this.state = { + list, + listForRendering: this.getListForRendering(list, rootApp) + }; + } else { + this.state = { + list: [], // array of apps ==> { name, instance } + listForRendering: [] // array of { name, Component } objects + }; + } + } + + getListForRendering(list, rootApp, listForRendering = []) { + const { + uniqueKey, + data + } = this.props; + + return list + .map((item) => { + const { + name, + weight, + multi + } = item; + const isPresent = listForRendering.some((w) => { + return w.name === name; + }); + + // @TODO: take care of removal in streamed list too? + if (isPresent) { + return null; + } + + const regionArgs = uniqueKey + ? [this.props.name, uniqueKey] + : [this.props.name]; + + if ( + uniqueKey && + !rootApp.hasAppInstance(name, ...regionArgs) + ) { + rootApp.instantiateApp(name, ...regionArgs); + } + + const instance = rootApp.getAppInstance(name, ...regionArgs); + + if (instance) { + Region.sendProps(instance, { + name: this.props.name, + uniqueKey, + data, + }); + } + + return { + name, + weight, + instance, + multi, + Component: getMountableComponent(instance), + }; + }) + .filter(item => !!item) + .concat(listForRendering) + .sort((a, b) => a.weight - b.weight); } shouldComponentUpdate(nextProps, nextState) { - return this._handler.shouldUpdate(nextProps, nextState); + let shouldUpdate = !isEqual(this.props, nextProps); + + if (!shouldUpdate) { + const { listForRendering } = nextState; + shouldUpdate = shouldUpdate || this.state.listForRendering.length !== listForRendering.length; + shouldUpdate = shouldUpdate || + zipWith(this.state.listForRendering, listForRendering, (a, b) => a.name === b.name) + .some(value => !value); + } + + return shouldUpdate; } - componentWillMount() { - this._handler.app = this.context.app; - this._handler.beforeMount(); + componentDidMount() { + if (!this.context.app) { + return; + } + + const rootApp = this.context.app.getRootApp(); + + this.rootApp = rootApp; + const apps$ = rootApp.getApps$( + this.props.name, + this.props.uniqueKey + ); + + this._subscription = apps$.subscribe({ + next: (list) => { + this.setState({ + list, + listForRendering: this.getListForRendering(list, rootApp, this.state.listForRendering) + }); + }, + error: (err) => { + console.warn(`Subscription error for :`, err); + } + }); } - componentWillReceiveProps(nextProps) { - this._handler.afterUpdate(nextProps); + componentWillReceiveProps(nextProps = {}) { + const { + name = this.props.name, + uniqueKey = this.props.uniqueKey, + data = this.props.data, + } = nextProps; + + this.state.listForRendering + .filter(item => item.instance) + .forEach(item => Region.sendProps(item.instance, { + name, + uniqueKey, + data, + })); } componentWillUnmount() { - this._handler.beforeDestroy(); + if (this._subscription) { + this._subscription.unsubscribe(); + } + + if (this.rootApp) { + this.state.listForRendering + .filter(item => item.multi) + .forEach((item) => { + this.rootApp.destroyApp( + item.name, + this.props.name, + this.props.uniqueKey + ); + }); + } } render() { diff --git a/packages/frint-react/src/components/Region.spec.js b/packages/frint-react/src/components/Region.spec.js index c6e672db..28cba6f5 100644 --- a/packages/frint-react/src/components/Region.spec.js +++ b/packages/frint-react/src/components/Region.spec.js @@ -13,6 +13,7 @@ import render from '../render'; import observe from './observe'; import Region from './Region'; import RegionService from '../services/Region'; +import renderToString from '../../../frint-react-server/src/renderToString'; import streamProps from '../streamProps'; describe('frint-react › components › Region', function () { @@ -400,4 +401,61 @@ describe('frint-react › components › Region', function () { expect(paragraph.parentElement.className).to.equal(className); expect(paragraph.innerHTML).to.equal('App 1'); }); + + it('should render when renderToString is called', function () { + // root + function RootComponent() { + return ( +
+ +
+ ); + } + const RootApp = createApp({ + name: 'RootApp', + providers: [ + { name: 'component', useValue: RootComponent }, + ], + }); + + // apps + function App1Component() { + return

App 1

; + } + const App1 = createApp({ + name: 'App1', + providers: [ + { name: 'component', useValue: App1Component }, + ], + }); + + function App2Component() { + return

App 2

; + } + const App2 = createApp({ + name: 'App2', + providers: [ + { name: 'component', useValue: App2Component }, + ], + }); + + // render + const rootApp = new RootApp(); + + // register apps + rootApp.registerApp(App1, { + regions: ['sidebar'], + weight: 10, + }); + + rootApp.registerApp(App2, { + regions: ['sidebar2'], + weight: 10, + }); + + const string = renderToString(rootApp); + + // verify + expect(string).to.include('App 1'); + }); }); From fd4c794d83db83bba9dfaf7040e983454360afb6 Mon Sep 17 00:00:00 2001 From: Javier Campa Lopez Date: Wed, 11 Jul 2018 15:38:48 +0200 Subject: [PATCH 2/7] Added additional check on tests --- packages/frint-react/src/components/Region.spec.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/frint-react/src/components/Region.spec.js b/packages/frint-react/src/components/Region.spec.js index 28cba6f5..58e8ec63 100644 --- a/packages/frint-react/src/components/Region.spec.js +++ b/packages/frint-react/src/components/Region.spec.js @@ -457,5 +457,6 @@ describe('frint-react › components › Region', function () { // verify expect(string).to.include('App 1'); + expect(string).not.to.include('App 2'); }); }); From f8947246c467ad54febdfbf4d0693551eb1a4865 Mon Sep 17 00:00:00 2001 From: Javier Campa Lopez Date: Wed, 11 Jul 2018 15:48:10 +0200 Subject: [PATCH 3/7] Added test in renderToString method --- .../src/renderToString.spec.js | 59 +++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/packages/frint-react-server/src/renderToString.spec.js b/packages/frint-react-server/src/renderToString.spec.js index f13a7cc7..29562cc1 100644 --- a/packages/frint-react-server/src/renderToString.spec.js +++ b/packages/frint-react-server/src/renderToString.spec.js @@ -7,6 +7,7 @@ import { createApp } from 'frint'; import { observe, streamProps } from 'frint-react'; import renderToString from './renderToString'; +import Region from '../../frint-react/src/components/Region'; describe('frint-react-server › renderToString', function () { it('is a function', function () { @@ -71,4 +72,62 @@ describe('frint-react-server › renderToString', function () { const html = renderToString(app); expect(html).to.contain('>TestAppName

'); }); + + it('returns HTML output of an App instance, with childs Apps', function () { + // root + function RootComponent() { + return ( +
+ +
+ ); + } + const RootApp = createApp({ + name: 'RootApp', + providers: [ + { name: 'component', useValue: RootComponent }, + ], + }); + + // apps + function App1Component() { + return

App 1

; + } + const App1 = createApp({ + name: 'App1', + providers: [ + { name: 'component', useValue: App1Component }, + ], + }); + + function App2Component() { + return

App 2

; + } + const App2 = createApp({ + name: 'App2', + providers: [ + { name: 'component', useValue: App2Component }, + ], + }); + + // render + const rootApp = new RootApp(); + + // register apps + rootApp.registerApp(App1, { + regions: ['sidebar'], + weight: 10, + }); + + rootApp.registerApp(App2, { + regions: ['sidebar2'], + weight: 10, + }); + + const string = renderToString(rootApp); + + // verify + expect(string).to.include('>App 1

'); + expect(string).not.to.include('>App 2

'); + }); }); From fc80fb48d84cf654d6189e21ad5937efa1462230 Mon Sep 17 00:00:00 2001 From: Javier Campa Lopez Date: Wed, 11 Jul 2018 16:16:25 +0200 Subject: [PATCH 4/7] Fixed import path from renderToString --- packages/frint-react-server/src/renderToString.spec.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/frint-react-server/src/renderToString.spec.js b/packages/frint-react-server/src/renderToString.spec.js index 29562cc1..209b39c9 100644 --- a/packages/frint-react-server/src/renderToString.spec.js +++ b/packages/frint-react-server/src/renderToString.spec.js @@ -4,10 +4,9 @@ import React from 'react'; import { expect } from 'chai'; import { createApp } from 'frint'; -import { observe, streamProps } from 'frint-react'; +import { observe, streamProps, Region } from 'frint-react'; import renderToString from './renderToString'; -import Region from '../../frint-react/src/components/Region'; describe('frint-react-server › renderToString', function () { it('is a function', function () { From db217e79b7c7e46b39b2bd995cdf9f24934db6d2 Mon Sep 17 00:00:00 2001 From: Javier Campa Lopez Date: Wed, 11 Jul 2018 17:05:12 +0200 Subject: [PATCH 5/7] Added test to increase coverage --- packages/frint-react/src/components/Region.js | 2 +- .../frint-react/src/components/Region.spec.js | 20 +++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/packages/frint-react/src/components/Region.js b/packages/frint-react/src/components/Region.js index df33606e..9cab27a8 100644 --- a/packages/frint-react/src/components/Region.js +++ b/packages/frint-react/src/components/Region.js @@ -145,7 +145,7 @@ export default class Region extends React.Component { }); } - componentWillReceiveProps(nextProps = {}) { + componentWillReceiveProps(nextProps) { const { name = this.props.name, uniqueKey = this.props.uniqueKey, diff --git a/packages/frint-react/src/components/Region.spec.js b/packages/frint-react/src/components/Region.spec.js index 58e8ec63..67e7ff10 100644 --- a/packages/frint-react/src/components/Region.spec.js +++ b/packages/frint-react/src/components/Region.spec.js @@ -459,4 +459,24 @@ describe('frint-react › components › Region', function () { expect(string).to.include('App 1'); expect(string).not.to.include('App 2'); }); + + it('should unmount component when no root app is available', function () { + function MyComponent() { + return ( +
+ +
+ ); + } + + ReactDOM.render( + , + document.getElementById('root') + ); + + const element = document.getElementById('my-component'); + expect(element.innerHTML).to.eql(''); + expect(ReactDOM.unmountComponentAtNode(document.getElementById('root'))).to.equal(true); + expect(document.getElementById('my-component')).to.equal(null); + }); }); From 9440937c6ec6ed0f388cb3ddb9c44d911701ee61 Mon Sep 17 00:00:00 2001 From: Javier Campa Lopez Date: Thu, 12 Jul 2018 10:19:41 +0200 Subject: [PATCH 6/7] Added tests for React Handler --- .../src/handlers/ReactHandler.spec.js | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) create mode 100644 packages/frint-react/src/handlers/ReactHandler.spec.js diff --git a/packages/frint-react/src/handlers/ReactHandler.spec.js b/packages/frint-react/src/handlers/ReactHandler.spec.js new file mode 100644 index 00000000..7f8919d0 --- /dev/null +++ b/packages/frint-react/src/handlers/ReactHandler.spec.js @@ -0,0 +1,42 @@ +/* eslint-disable import/no-extraneous-dependencies, func-names */ +/* global describe, it */ +import { expect } from 'chai'; + +import { composeHandlers } from 'frint-component-utils'; + +import ReactHandler from './ReactHandler'; + +describe('frint-react › ReactHandler', function () { + const component = { + state: { + text: '' + }, + setState(newState, cb) { + this.state = { ...this.state, ...newState }; + cb && cb(); + }, + props: { + value: 'hi' + } + }; + + it('is an object', function () { + expect(ReactHandler).to.be.an('object'); + }); + + it('gets and sets data from component', function () { + const handler = composeHandlers( + ReactHandler, + { + component + }, + ); + + handler.setData('text', 'hello'); + expect(handler.getData('text')).to.equal('hello'); + handler.setDataWithCallback('text', 'hello1', () => {}); + expect(handler.getData('text')).to.equal('hello1'); + expect(handler.getProps()).to.have.property('value', 'hi'); + expect(handler.getProp('value')).to.equal('hi'); + }); +}); From 767bed688200d6b52aa257f210be236ee2389f04 Mon Sep 17 00:00:00 2001 From: Javier Campa Lopez Date: Thu, 12 Jul 2018 10:56:35 +0200 Subject: [PATCH 7/7] Excluded lib folder from coverage report --- packages/frint-react/package.json | 8 +++++++- packages/frint-react/src/components/Region.spec.js | 3 ++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/packages/frint-react/package.json b/packages/frint-react/package.json index 2241a537..bb91642a 100644 --- a/packages/frint-react/package.json +++ b/packages/frint-react/package.json @@ -34,11 +34,17 @@ "cross-env": "^5.0.5", "frint": "^5.5.0", "frint-config": "^5.5.0", - "frint-test-utils": "^5.5.0" + "frint-test-utils": "^5.5.0", + "frint-react-server": "^5.5.0" }, "bugs": { "url": "https://github.com/frintjs/frint/issues" }, + "nyc": { + "exclude": [ + "lib" + ] + }, "license": "MIT", "types": "index.d.ts" } diff --git a/packages/frint-react/src/components/Region.spec.js b/packages/frint-react/src/components/Region.spec.js index 67e7ff10..6abe5df6 100644 --- a/packages/frint-react/src/components/Region.spec.js +++ b/packages/frint-react/src/components/Region.spec.js @@ -8,12 +8,13 @@ import { Subject } from 'rxjs/Subject'; import sinon from 'sinon'; import { createApp } from 'frint'; +import { renderToString } from 'frint-react-server'; import render from '../render'; import observe from './observe'; import Region from './Region'; import RegionService from '../services/Region'; -import renderToString from '../../../frint-react-server/src/renderToString'; + import streamProps from '../streamProps'; describe('frint-react › components › Region', function () {