From 3896651ed50408149b8080c5b292c2d4e3de365c Mon Sep 17 00:00:00 2001 From: Spencer Brown Date: Wed, 14 Jun 2017 11:21:38 -0700 Subject: [PATCH 1/3] feat: Update state when props passed to connected components change Fixes https://github.com/mongodb-js/connect-backbone-to-react/issues/6. --- lib/connect-backbone-to-react.js | 16 ++++++++-- test/connect-backbone-to-react.test.js | 43 ++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 3 deletions(-) diff --git a/lib/connect-backbone-to-react.js b/lib/connect-backbone-to-react.js index 65c4b78..2ce78c8 100644 --- a/lib/connect-backbone-to-react.js +++ b/lib/connect-backbone-to-react.js @@ -67,9 +67,8 @@ module.exports = function connectBackboneToReact( constructor(props, context) { super(props, context); - this.models = props.models || context.models; - - validateModelTypes(this.models); + const models = props.models || context.models; + this.setModels(models); this.state = mapModelsToProps(this.models); @@ -83,6 +82,11 @@ module.exports = function connectBackboneToReact( this.createEventListeners(); } + setModels(models) { + validateModelTypes(models); + this.models = models; + } + createEventListeners() { Object.keys(this.models).forEach(mapKey => { const model = this.models[mapKey]; @@ -105,6 +109,12 @@ module.exports = function connectBackboneToReact( this.setState(mapModelsToProps(this.models)); } + componentWillReceiveProps(nextProps, nextContext) { + const models = nextProps.models || nextContext.models; + this.setModels(models); + this.createNewProps(); + } + componentWillUnmount() { if (debounce) { this.createNewProps.cancel(); diff --git a/test/connect-backbone-to-react.test.js b/test/connect-backbone-to-react.test.js index bbcd681..ef6b306 100644 --- a/test/connect-backbone-to-react.test.js +++ b/test/connect-backbone-to-react.test.js @@ -473,6 +473,49 @@ describe('connectBackboneToReact', function() { }); }); + describe('when passed props change', function() { + let setStateSpy; + let newName; + let newAge; + + beforeEach(function() { + const ConnectedTest = connectBackboneToReact(mapModelsToProps)(TestComponent); + setStateSpy = sandbox.spy(ConnectedTest.prototype, 'setState'); + + wrapper = mount(); + stub = wrapper.find(TestComponent); + + newName = 'Robert'; + newAge = '30'; + + const newUserModel = new UserModel({ + name: newName, + age: newAge, + hungry: false, + }); + const newModelsMap = { + user: newUserModel, + coll: userCollection, + }; + + wrapper.setProps({ models: newModelsMap }); + }); + + afterEach(function() { + wrapper.unmount(); + }); + + it('calls setState once', function() { + assert.equal(setStateSpy.calledOnce, true); + }); + + it('renders the new props', function() { + assert.equal(stub.find('.name').text(), newName); + assert.equal(stub.find('.age').text(), newAge); + assert.equal(stub.find('.hungry').text(), 'not hungry'); + }); + }); + describe('when unmounted in an event listener and subscribed to "all" event', function() { // To add more color, "all" event handlers are triggered after individual event handlers. // That is to say, if you trigger "foo" the sequence of event handlers called is: From e01f5e759d0657908eba97b917fb44d54b37123c Mon Sep 17 00:00:00 2001 From: Garret Meier Date: Wed, 2 May 2018 10:04:54 -0700 Subject: [PATCH 2/3] Prototype using the React 16.3 Context API In [React 16.3](https://reactjs.org/blog/2018/03/29/react-v-16-3.html#official-context-api) they announced a new, official Context API. Here we can use this api to simplify the `BackboneProvider` and the `connectBackboneToReact` function. We no longer need to use soon to be deprecated lifecycle methods and can instead just set up listeners for backbone events and map models to props as previously. Fortunately, we can accomplish this without changes to the `connect-backbone-to-react` API, but unfortunately, it requires updating to `react@^16.3.0` (along with `prop-types` and `react-dom`). This also means that tests have to be updated to use the latest version of `enzyme` which doesn't [yet support](https://github.com/airbnb/enzyme/issues/1553) the new Context API. Until then, we can leave this as a prototype. --- lib/backbone-provider.js | 21 +++++---- lib/connect-backbone-to-react.js | 77 +++++++++++++------------------- lib/context.js | 2 + package.json | 13 +++--- test/test-setup.js | 3 ++ 5 files changed, 56 insertions(+), 60 deletions(-) create mode 100644 lib/context.js diff --git a/lib/backbone-provider.js b/lib/backbone-provider.js index 9b0236e..435be3e 100644 --- a/lib/backbone-provider.js +++ b/lib/backbone-provider.js @@ -1,15 +1,23 @@ -const { Component, Children } = require('react'); +const React = require('react'); +const {Children, Component} = React; const PropTypes = require('prop-types'); +const ConnectBackboneToReactContext = require('./context.js'); // eslint-disable-line no-unused-vars class BackboneProvider extends Component { - getChildContext() { - return { - models: this.props.models, + constructor(props) { + super(props); + + this.state = { + models: props.models, }; } render() { - return Children.only(this.props.children); + return ( + + {Children.only(this.props.children)} + + ); } } @@ -17,9 +25,6 @@ BackboneProvider.propTypes = { models: PropTypes.object, children: PropTypes.element.isRequired, }; -BackboneProvider.childContextTypes = { - models: PropTypes.object, -}; BackboneProvider.displayName = 'BackboneProvider'; module.exports = BackboneProvider; diff --git a/lib/connect-backbone-to-react.js b/lib/connect-backbone-to-react.js index 0d28df9..4742628 100644 --- a/lib/connect-backbone-to-react.js +++ b/lib/connect-backbone-to-react.js @@ -1,7 +1,9 @@ const hoistStatics = require('hoist-non-react-statics'); -const { Component, createElement } = require('react'); +const React = require('react'); +const {Component} = React; const PropTypes = require('prop-types'); const debounceFn = require('lodash.debounce'); +const ConnectBackboneToReactContext = require('./context.js'); // eslint-disable-line no-unused-vars function getDisplayName(name) { return `connectBackboneToReact(${name})`; @@ -28,7 +30,7 @@ module.exports = function connectBackboneToReact( const { debounce = false, events = {}, - modelTypes = {}, + modelTypes = {} } = options; function getEventNames(modelName) { @@ -66,39 +68,26 @@ module.exports = function connectBackboneToReact( const displayName = getDisplayName(wrappedComponentName); class ConnectBackboneToReact extends Component { - constructor(props, context) { - super(props, context); + constructor(props) { + super(props); - const models = props.models || context.models; - this.setModels(models); - - this.state = mapModelsToProps(this.models, this.props); + this.models = {}; + this.state = {}; this.createNewProps = this.createNewProps.bind(this); + this.renderChild = this.renderChild.bind(this); if (debounce) { const debounceWait = typeof debounce === 'number' ? debounce : 0; this.createNewProps = debounceFn(this.createNewProps, debounceWait); } - - this.createEventListeners(); } - setModels(models) { + setModels(models = {}) { validateModelTypes(models); this.models = models; } - createEventListeners() { - Object.keys(this.models).forEach(mapKey => { - const model = this.models[mapKey]; - // Do not attempt to create event listeners on an undefined model. - if (!model) return; - - this.createEventListener(mapKey, model); - }); - } - createEventListener(modelName, model) { getEventNames(modelName).forEach(name => { model.on(name, this.createNewProps, this); @@ -110,33 +99,34 @@ module.exports = function connectBackboneToReact( // The only case where this flag is encountered is when this component // is unmounted within an event handler but the 'all' event is still triggered. // It is covered in a test case. - if (this.hasBeenUnmounted) { - return; - } + if (this.hasBeenUnmounted) return; this.setState(mapModelsToProps(this.models, this.props)); } - componentWillReceiveProps(nextProps, nextContext) { - this.setModels(nextProps, nextContext); - this.createNewProps(); - + renderChild({ models } = {}) { + const newModels = Object.assign({}, models, this.props.models, this.models); // Bind event listeners for each model that changed. - Object.keys(this.models).forEach(mapKey => { - const model = this.models[mapKey]; - if ((this.props.models && this.props.models[mapKey] === this.models[mapKey]) || - (this.context.models && this.context.models[mapKey] === this.models[mapKey])) { + Object.keys(newModels).forEach(mapKey => { + const model = newModels[mapKey]; + if (this.models && this.models[mapKey] === newModels[mapKey]) { return; // Did not change. } this.createEventListener(mapKey, model); + this.models[mapKey] = model; }); - } + validateModelTypes(this.models); - componentWillReceiveProps(nextProps, nextContext) { - const models = nextProps.models || nextContext.models; - this.setModels(models); - this.createNewProps(); + const wrappedProps = Object.assign( + {}, + mapModelsToProps(this.models, this.props), + this.props + ); + + // Don't pass through models prop. + wrappedProps.models = undefined; + return React.createElement(WrappedComponent, wrappedProps); } componentWillUnmount() { @@ -158,16 +148,11 @@ module.exports = function connectBackboneToReact( } render() { - const wrappedProps = Object.assign( - {}, - this.state, - this.props + return ( + + {this.renderChild} + ); - - // Don't pass through models prop. - wrappedProps.models = undefined; - - return createElement(WrappedComponent, wrappedProps); } } diff --git a/lib/context.js b/lib/context.js new file mode 100644 index 0000000..2a6f24f --- /dev/null +++ b/lib/context.js @@ -0,0 +1,2 @@ +const React = require('react'); +module.exports = React.createContext('connectBackboneToReact'); diff --git a/package.json b/package.json index c91ed0a..c240806 100644 --- a/package.json +++ b/package.json @@ -27,10 +27,10 @@ "dependencies": { "hoist-non-react-statics": "^1.2.0", "lodash.debounce": "^4.0.8", - "prop-types": "^15.5.8" + "prop-types": "^15.6.1" }, "peerDependencies": { - "react": "^0.14.0 || ^15.0.0-0 || ^16.0.0-0" + "react": "^16.0.0-0" }, "devDependencies": { "babel-cli": "^6.22.2", @@ -38,16 +38,17 @@ "babel-preset-react": "^6.22.0", "babel-register": "^6.22.0", "backbone": "^1.3.3", - "enzyme": "^2.7.1", + "enzyme": "^3.3.0", + "enzyme-adapter-react-16": "^1.1.1", "eslint-config-mongodb-js": "^2.2.0", "jsdom": "^9.11.0", "mocha": "^3.2.0", "mongodb-js-fmt": "^0.0.3", "mongodb-js-precommit": "^0.2.8", "pre-commit": "^1.1.2", - "react": "^15.4.2", - "react-addons-test-utils": "^15.4.2", - "react-dom": "^15.4.2", + "react": "^16.3.2", + "react-dom": "^16.3.2", + "react-test-renderer": "^16.3.2", "sinon": "^1.17.7", "standard-version": "^4.0.0" }, diff --git a/test/test-setup.js b/test/test-setup.js index d63b469..1f9180b 100644 --- a/test/test-setup.js +++ b/test/test-setup.js @@ -1,4 +1,7 @@ require('babel-register'); +const Enzyme = require('enzyme'); +const Adapter = require('enzyme-adapter-react-16'); +Enzyme.configure({ adapter: new Adapter() }); const jsdom = require('jsdom').jsdom; global.document = jsdom(''); From 3ae64e67ab70a08430ec89c7177eea116ede2dab Mon Sep 17 00:00:00 2001 From: Garret Meier Date: Sat, 7 Dec 2019 16:19:24 -0800 Subject: [PATCH 3/3] improvement: use latest react and fix tests This fixes the tests for CBR when it uses react context. --- lib/backbone-provider.js | 5 +++ lib/connect-backbone-to-react.js | 12 +++---- package.json | 14 ++++---- test/backbone-provider.test.js | 10 +++--- test/connect-backbone-to-react.test.js | 50 +++++++++++++------------- 5 files changed, 49 insertions(+), 42 deletions(-) diff --git a/lib/backbone-provider.js b/lib/backbone-provider.js index 435be3e..303b92f 100644 --- a/lib/backbone-provider.js +++ b/lib/backbone-provider.js @@ -12,6 +12,11 @@ class BackboneProvider extends Component { }; } + componentDidUpdate(prevProps) { + if (this.props.models === prevProps.models) return; + this.setState({ models: this.props.models }); + } + render() { return ( diff --git a/lib/connect-backbone-to-react.js b/lib/connect-backbone-to-react.js index 4742628..bdda2d4 100644 --- a/lib/connect-backbone-to-react.js +++ b/lib/connect-backbone-to-react.js @@ -30,7 +30,7 @@ module.exports = function connectBackboneToReact( const { debounce = false, events = {}, - modelTypes = {} + modelTypes = {}, } = options; function getEventNames(modelName) { @@ -100,12 +100,12 @@ module.exports = function connectBackboneToReact( // is unmounted within an event handler but the 'all' event is still triggered. // It is covered in a test case. if (this.hasBeenUnmounted) return; - this.setState(mapModelsToProps(this.models, this.props)); } - renderChild({ models } = {}) { - const newModels = Object.assign({}, models, this.props.models, this.models); + renderChild({ models } = {}) { + const newModels = Object.assign({}, this.models, models, this.props.models); + // Bind event listeners for each model that changed. Object.keys(newModels).forEach(mapKey => { const model = newModels[mapKey]; @@ -113,7 +113,7 @@ module.exports = function connectBackboneToReact( return; // Did not change. } - this.createEventListener(mapKey, model); + if (model) this.createEventListener(mapKey, model); this.models[mapKey] = model; }); validateModelTypes(this.models); @@ -125,7 +125,7 @@ module.exports = function connectBackboneToReact( ); // Don't pass through models prop. - wrappedProps.models = undefined; + delete wrappedProps.models; return React.createElement(WrappedComponent, wrappedProps); } diff --git a/package.json b/package.json index c240806..df11787 100644 --- a/package.json +++ b/package.json @@ -27,10 +27,10 @@ "dependencies": { "hoist-non-react-statics": "^1.2.0", "lodash.debounce": "^4.0.8", - "prop-types": "^15.6.1" + "prop-types": "^15.7.2" }, "peerDependencies": { - "react": "^16.0.0-0" + "react": "^16.3.0-0" }, "devDependencies": { "babel-cli": "^6.22.2", @@ -38,16 +38,16 @@ "babel-preset-react": "^6.22.0", "babel-register": "^6.22.0", "backbone": "^1.3.3", - "enzyme": "^3.3.0", - "enzyme-adapter-react-16": "^1.1.1", - "eslint-config-mongodb-js": "^2.2.0", + "enzyme": "^3.10.0", + "enzyme-adapter-react-16": "^1.15.1", + "eslint-config-mongodb-js": "^2.3.0", "jsdom": "^9.11.0", "mocha": "^3.2.0", "mongodb-js-fmt": "^0.0.3", "mongodb-js-precommit": "^0.2.8", "pre-commit": "^1.1.2", - "react": "^16.3.2", - "react-dom": "^16.3.2", + "react": "^16.12.0", + "react-dom": "^16.12.0", "react-test-renderer": "^16.3.2", "sinon": "^1.17.7", "standard-version": "^4.0.0" diff --git a/test/backbone-provider.test.js b/test/backbone-provider.test.js index 6ab5381..17cb173 100644 --- a/test/backbone-provider.test.js +++ b/test/backbone-provider.test.js @@ -81,7 +81,7 @@ describe('BackboneProvider', function() { }); afterEach(function() { - wrapper.unmount(); + if (wrapper.exists()) wrapper.unmount(); }); it('passes mapped models and collections as properties to wrapped component', function() { @@ -139,11 +139,11 @@ describe('BackboneProvider', function() { const modelsFromContext = wrapper .find('.name') - .findWhere((n) => n.text() === userModel.get('name')) + .findWhere((n) => !n.type() && n.text() === userModel.get('name')) .length; const modelsFromParent = wrapper .find('.color') - .findWhere((n) => n.text() === settingsModel.get('color')) + .findWhere((n) => !n.type() && n.text() === settingsModel.get('color')) .length; // Check that we've rendered data from models passed by both context and the parent component. @@ -186,12 +186,12 @@ describe('BackboneProvider', function() { const modelsFromContext = wrapper .find('.name') - .findWhere((n) => n.text() === userModel.get('name')) + .findWhere((n) => !n.type() && n.text() === userModel.get('name')) .length; const modelsFromParent = wrapper .find('.child-wrapper') .find('.name') - .findWhere((n) => n.text() === otherUserModel.get('name')) + .findWhere((n) => !n.type() && n.text() === otherUserModel.get('name')) .length; // Check that we've given priority to models passed from the parent component. diff --git a/test/connect-backbone-to-react.test.js b/test/connect-backbone-to-react.test.js index f2b829a..b4eeab3 100644 --- a/test/connect-backbone-to-react.test.js +++ b/test/connect-backbone-to-react.test.js @@ -96,7 +96,7 @@ describe('connectBackboneToReact', function() { }); afterEach(function() { - wrapper.unmount(); + if (wrapper.exists()) wrapper.unmount(); }); it('passes mapped models and collections as properties to wrapped component', function() { @@ -115,9 +115,10 @@ describe('connectBackboneToReact', function() { it('updates properties when props function changes models and collections ', function() { const newName = 'The Loud One'; - stub.props().changeName(newName); + stub.prop('changeName')(newName); + wrapper.update(); assert.equal(userModel.get('name'), newName); - assert.equal(stub.props().name, newName); + assert.equal(wrapper.find(TestComponent).prop('name'), newName); assert.equal(setStateSpy.callCount, 4); }); @@ -125,9 +126,10 @@ describe('connectBackboneToReact', function() { it('updates properties when model and collections change', function() { const newName = 'Banana'; userModel.set('name', newName); + wrapper.update(); assert.equal(wrapper.find('.name').text(), 'Banana'); assert.equal(userModel.get('name'), newName); - assert.equal(stub.props().name, newName); + assert.equal(wrapper.find(TestComponent).prop('name'), newName); assert.equal(setStateSpy.callCount, 4); }); @@ -169,7 +171,7 @@ describe('connectBackboneToReact', function() { }); afterEach(function() { - wrapper.unmount(); + if (wrapper.exists()) wrapper.unmount(); }); it('updates properties when model and collections change', function(done) { @@ -177,9 +179,10 @@ describe('connectBackboneToReact', function() { userModel.set('name', newName); setTimeout(() => { + wrapper.update(); assert.equal(wrapper.find('.name').text(), 'Banana'); assert.equal(userModel.get('name'), newName); - assert.equal(stub.props().name, newName); + assert.equal(wrapper.find(TestComponent).prop('name'), newName); assert.equal(setStateSpy.callCount, 1); @@ -202,7 +205,7 @@ describe('connectBackboneToReact', function() { describe('when mounted with an undefined model', function() { afterEach(function() { - wrapper.unmount(); + if (wrapper.exists()) wrapper.unmount(); }); it('the default should mount and unmount the component successfully', function() { @@ -232,7 +235,7 @@ describe('connectBackboneToReact', function() { }); afterEach(function() { - wrapper.unmount(); + if (wrapper.exists()) wrapper.unmount(); }); it('sets one event handler on the userModel', function() { @@ -247,9 +250,10 @@ describe('connectBackboneToReact', function() { it('updates properties when model\'s name changes', function() { const newName = 'Banana'; userModel.set('name', newName); + wrapper.update(); assert.equal(userModel.get('name'), newName); - assert.equal(stub.props().name, newName); + assert.equal(wrapper.find(TestComponent).prop('name'), newName); }); it('rerenders when tracked property changes', function() { @@ -291,7 +295,7 @@ describe('connectBackboneToReact', function() { }); afterEach(function() { - wrapper.unmount(); + if (wrapper.exists()) wrapper.unmount(); }); it('sets 0 event handlers on the userModel', function() { @@ -318,7 +322,7 @@ describe('connectBackboneToReact', function() { }); afterEach(function() { - wrapper.unmount(); + if (wrapper.exists()) wrapper.unmount(); }); it('passes connectedProps through', function() { @@ -342,7 +346,7 @@ describe('connectBackboneToReact', function() { }); afterEach(function() { - wrapper.unmount(); + if (wrapper.exists()) wrapper.unmount(); }); it('uses default mapModelsToProps function', function() { @@ -362,8 +366,9 @@ describe('connectBackboneToReact', function() { it('re-renders props when model changes', function() { const newName = 'Banana'; userModel.set('name', newName); + wrapper.update(); - assert.equal(stub.props().user.name, 'Banana'); + assert.equal(wrapper.find(TestComponent).prop('user').name, 'Banana'); assert.equal(setStateSpy.callCount, 4); }); @@ -388,7 +393,7 @@ describe('connectBackboneToReact', function() { }); afterEach(function() { - wrapper.unmount(); + if (wrapper.exists()) wrapper.unmount(); }); it('uses default mapModelsToProps function', function() { @@ -409,8 +414,9 @@ describe('connectBackboneToReact', function() { it('re-renders props when model changes', function() { const newName = 'Banana'; userModel.set('name', newName); + wrapper.update(); - assert.equal(stub.props().user.name, 'Banana'); + assert.equal(wrapper.find(TestComponent).prop('user').name, 'Banana'); assert.equal(setStateSpy.callCount, 1); }); @@ -523,7 +529,7 @@ describe('connectBackboneToReact', function() { }); afterEach(function() { - wrapper.unmount(); + if (wrapper.exists()) wrapper.unmount(); }); it('retrieves the correct model based on props', function() { @@ -544,15 +550,15 @@ describe('connectBackboneToReact', function() { }); describe('when passed props change', function() { - let setStateSpy; let newName; let newAge; let newUserModel; beforeEach(function() { + // Disable no-unused-vars on the next line because the current version doesn't + // detect that is a usage. + // eslint-disable-next-line const ConnectedTest = connectBackboneToReact(mapModelsToProps)(TestComponent); - setStateSpy = sandbox.spy(ConnectedTest.prototype, 'setState'); - wrapper = mount(); stub = wrapper.find(TestComponent); @@ -573,11 +579,7 @@ describe('connectBackboneToReact', function() { }); afterEach(function() { - wrapper.unmount(); - }); - - it('calls setState once', function() { - assert.equal(setStateSpy.calledOnce, true); + if (wrapper.exists()) wrapper.unmount(); }); it('renders the new props', function() {