-
Notifications
You must be signed in to change notification settings - Fork 32
How about just intercepting setModel? #22
Comments
OK, since this was making me itch, I've figured out a way that is potentially more elegant. Essentially, So, the solution seems to abstract out the model state handling instead. In fact, in all likelihood this is what any middleware is interested in anyway. The rest is solvable on a (form) component level. I've reworded Incidentally, it seems to me that it would make sense to do all this in the constructor instead of the import React from 'react'
import PropTypes from 'prop-types'
import assign from 'object-assign'
import hoistNonReactStatics from 'hoist-non-react-statics'
const makeWrapper = (middleware) => (WrappedComponent) => {
class FormWrapper extends React.Component {
static propTypes = {
initialModel: PropTypes.object,
}
constructor (props, ctx) {
super(props, ctx)
this.state = {
model: props.initialModel || {},
}
class ModelHandlerWrapper {
constructor({getState,setState}) {
this.getState = getState
this.setState = setState
}
setModel = (model) => {
this.setState({model})
return model
}
getModel = ()=> getState("model")
getModelValue = (name)=> this.getModel()[name]
setModelValue = (name,value)=> {
return this.setModel(assign({}, this.getModel(), {
[name]: value,
}))
}
}
const getState = (name) => this.state[name]
this.modelHandler = new ModelHandlerWrapper({getState: getState, setState: this.setState.bind(this)})
if (typeof middleware === 'function') {
this.modelHandler = middleware(this.modelHandler)
}
}
setModel = (model) => {
return this.modelHandler.setModel(model)
}
setProperty = (prop, value) => {
return this.modelHandler.setModelValue(prop,value)
}
// This, of course, does not handle all possible inputs. In such cases,
// you should just use `setProperty` or `setModel`. Or, better yet,
// extend `reformed` to supply the bindings that match your needs.
bindToChangeEvent = (e) => {
const { name, type, value } = e.target
if (type === 'checkbox') {
const oldCheckboxValue = this.modelHandler.getModelValue(name) || []
const newCheckboxValue = e.target.checked
? oldCheckboxValue.concat(value)
: oldCheckboxValue.filter(v => v !== value)
this.setProperty(name, newCheckboxValue)
} else {
this.setProperty(name, value)
}
}
bindInput = (name) => {
return {
name,
value: this.modelHandler.getModelValue(name) || '',
onChange: this.bindToChangeEvent,
}
}
render () {
let nextProps = assign({}, this.props, {
model: this.modelHandler.getModel(),
setProperty: this.setProperty,
setModel: this.setModel,
bindInput: this.bindInput,
bindToChangeEvent: this.bindToChangeEvent
})
return React.createElement(WrappedComponent, nextProps)
}
}
FormWrapper.displayName = `Reformed(${getComponentName(WrappedComponent)})`
return hoistNonReactStatics(FormWrapper, WrappedComponent)
}
const getComponentName = (component) => (
component.displayName ||
component.name
)
export default makeWrapper |
Yeah, I'd realized the patch wouldn't affect I will read through your proposals when I get some time today, but wanted to get a quick response out. Thank you for the detailed post. If it's a clean enough solution we can consider it :). |
I took a look at your proposal, and it seems pretty solid for your use case. I do have some reservations, though, mainly that the middleware function would only run once, at component instantiation time, which would break the expected function signature of At this point I'm wondering if this is something that middleware shouldn't even be responsible for, since the only reliable way to update function references after applying middleware is to have them rely on I would really like to solve for this though, so I'm continuing to play around with a few ideas on my end. If this is blocking you, though, please feel free to fork this project in the meantime :). |
Yes, I agree that it's an interesting (architectural) challenge. To be honest I'm not familiar enough with the React lifecycle and it's architectural consequences to make a good estimate here. However, it seems to me that it wouldn't be too hard to use this approach inside of the "render" cycle. I guess you would end up with some kind of generator functionality like you wrote before for the The current code is quite tied into |
OK, this seems to work fine and it allows the middleware to jump in during the You might note that I'm using import React from 'react'
import PropTypes from 'prop-types'
import assign from 'object-assign'
import hoistNonReactStatics from 'hoist-non-react-statics'
import { set, get } from 'lodash'
class ModelHandlerWrapper {
constructor({getState,setState}) {
this.getState = getState
this.setState = setState
}
setModel = (model) => {
this.setState({model})
return model
}
getModel = ()=> this.getState("model")
getModelValue = (name)=> get(this.getModel(),name)
setModelValue = (name,value)=> {
return this.setModel(set(this.getModel(),name,value))
}
}
const makeWrapper = (middleware) => (WrappedComponent) => {
class FormWrapper extends React.Component {
static propTypes = {
initialModel: PropTypes.object,
}
constructor (props, ctx) {
super(props, ctx)
this.state = {
model: props.initialModel || {},
}
}
makeHelpers = (modelHandler) => {
bindToChangeEvent = (e) => {
const { name, type, value } = e.target
if (type === 'checkbox') {
const oldCheckboxValue = modelHandler.getModelValue(name) || []
const newCheckboxValue = e.target.checked
? oldCheckboxValue.concat(value)
: oldCheckboxValue.filter(v => v !== value)
modelHandler.setModelValue(name, newCheckboxValue)
} else {
modelHandler.setModelValue(name, value)
}
}
helpers = {
setModel: (model) => modelHandler.setModel(model)
,
setProperty: (prop, value) => modelHandler.setModelValue(prop,value)
,
bindToChangeEvent: (e) => {
const { name, type, value } = e.target
if (type === 'checkbox') {
const oldCheckboxValue = modelHandler.getModelValue(name) || []
const newCheckboxValue = e.target.checked
? oldCheckboxValue.concat(value)
: oldCheckboxValue.filter(v => v !== value)
modelHandler.setModelValue(name, newCheckboxValue)
} else {
modelHandler.setModelValue(name, value)
}
}
,
bindInput: (name) => {
return {
name,
value: modelHandler.getModelValue(name) || '',
onChange: bindToChangeEvent,
}
}
}
return helpers
}
render () {
const getState = (name) => this.state[name]
let modelHandler = new ModelHandlerWrapper({getState: getState, setState: this.setState.bind(this)})
if (typeof middleware === 'function') {
modelHandler = middleware(modelHandler)
}
let nextProps = assign({}, this.props, {
model: modelHandler.getModel()
},
this.makeHelpers(modelHandler)
)
return React.createElement(WrappedComponent, nextProps)
}
}
FormWrapper.displayName = `Reformed(${getComponentName(WrappedComponent)})`
return hoistNonReactStatics(FormWrapper, WrappedComponent)
}
const getComponentName = (component) => (
component.displayName ||
component.name
)
export default makeWrapper |
OK, I was a bit too fast in stating that 2.0.0 fixed my issue. My goal was to intercept
setModel
, notsetProperty
and it's still impossible to override. Now that I understand what is happening, it's easy to see why: if you don't overridesetProperty
, it will still be "locked in" with the originalsetModel
function.This for example will not work.
Now, you may begin to wonder why we would want to use that?
Let's say we're creating a dynamic form that has "repeatable" parts. The output we are aiming for is an array like this:
If I can intercept
setModel
, I can easily store the state of the entire "form" into an array inside of a higher order state object. We could perhaps do this withsetProperty
as well, but I'm betting this will breakbindInput
, etc because the internal state of the "lower order" reformed component will not be updated anymore...The text was updated successfully, but these errors were encountered: