Prototype React 16 context #19
Conversation
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](enzymejs/enzyme#1553) the new Context API. Until then, we can leave this as a prototype.
There was a problem hiding this comment.
Thank you for taking this on!
Definitely interested in adopting the new features of React 16 into this library so very excited to see this.
I'm seeing some existing eslint errors in the Travis build. Hoping if we get that fixed we can see if the tests can run at all.
I also have some initial feed back on this PR.
I think we'd release this as a new major version as it contains some breaking changes from how it works underneath the hood.
Again, thank you!
| const PropTypes = require('prop-types'); | ||
| const ConnectBackboneToReactContext = require('./context.js'); // eslint-disable-line no-unused-vars |
There was a problem hiding this comment.
nit, remove file extension.
Also is this eslint still needed?
| @@ -0,0 +1,2 @@ | |||
| const React = require('react'); | |||
| module.exports = React.createContext('connectBackboneToReact'); | |||
There was a problem hiding this comment.
The argument to createContext is the default value. Our default value is not a string. We can safely omit that.
| const PropTypes = require('prop-types'); | ||
| const debounceFn = require('lodash.debounce'); | ||
| const ConnectBackboneToReactContext = require('./context.js'); // eslint-disable-line no-unused-vars |
| } | ||
|
|
||
| setModels(props, context) { | ||
| const models = Object.assign({}, context.models, props.models); | ||
| setModels(models = {}) { |
There was a problem hiding this comment.
Looks like we don't use this method anymore.
| } | ||
|
|
||
| setModels(props, context) { | ||
| const models = Object.assign({}, context.models, props.models); | ||
| setModels(models = {}) { | ||
| validateModelTypes(models); | ||
| this.models = models; |
There was a problem hiding this comment.
I don't see this being updated ever either. I'm curious if we even need it anymore.
| }); | ||
| validateModelTypes(this.models); |
There was a problem hiding this comment.
We should be validating models before we create event listeners. If we don't have a test for this already we should add one.
| super(props); | ||
|
|
||
| this.state = { | ||
| models: props.models, | ||
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
We need to implement getDerivedStateFromProps to copy over new prop models when they are given.
Or I wonder if we could just use this.props as the value given to the provider.
This fixes the tests for CBR when it uses react context.
|
We managed to merge this in via #25. |
Hey, wanted to start the conversation on this since the current version of
connect-backbone-to-reactdoes not work when used with React 16.3's new context apis likegetSnapshotBeforeUpdate. This PR mimics existing functionality using the newProviderandConsumercomponents while removing soon-to-be-deprecated methods likecomponentWillReceivePropsas well as the old context lifecycle methods.Note that the tests here fail since the current version of
enzymedoesn't support the new context methods (the related issue is here: enzymejs/enzyme#1553).