Cannot use a higher-order function in Field Validation

[duro]

Are you submitting a bug report or a feature request?

Bug

What is the current behavior?

When using field level validators, I am unable to use a higher order function to produce the validation function

This works:

const required = value => (value ? undefined : 'Required')

<Field
  label="Email"
  name="email"
  component={InputField}
  validate={required}
  placeholder="email"
  type="text"
/>

This DOES NOT work:

const required = (msg = 'Default Message') => value => (value ? undefined : msg)

<Field
  label="Email"
  name="email"
  component={InputField}
  validate={required('Message Override')}
  placeholder="email"
  type="text"
/>

What is the expected behavior?

I would expect to pass into the field validator a returned function that matches the validator interface even if it was produced by a higher order function

Sandbox Link

https://codesandbox.io/s/G6ARklAMr

What's your environment?

Redux Form: 7.0.3
OS: macOS
Browser: Chrome 60

Other information

[duro]

Just reverted back to 6.8.0 and this works as expected.

[danielrob]

This is expected behaviour see #3271.

[duro]

@danielrob Cool. I followed through on the threads and updated docs, but I'll be honest. This is still a very confusing change to me. I guess I need to switch to using a bound method instead of a lambda created at render time, but I'm still not really sure.

[danielrob]

I fully admit my documentation update for this was minimal... and that while this change makes the library more powerful, it also makes the consumption more advanced.

Your help would be appreciated to alleviate this... perhaps you could think about where in the documentation / how this could be explained to make it clear?

Edit: I hadn't actually realised that this affects all inline validation functions regardless of whether they were higher order functions or not. See the other ticket.

[gabrielhpugliese]

Could you please enlighten me here? What's the cause for this not to work? I didn't get it yet.

[danielrob]

@gabrielhpugliese ok the latest:
You must define your validation functions outside of your render method. Additionally if they are a higher order function which receive props you should instantiate them when the component mounts / on instantiation.

So either (option 1):

const myvalidate = (validateParams) => ( /* some function on validateParams */)
const MyComponent = () => {
  <Field
    validate={myvalidate}
  />
}

Or something like (option 2):

const myValidate = config => (validateParams) => ( /* some function of validateParams & config */ )

class MyComponent extends React.Component {
  constructor(props) {
    super(props)
    this.validate = myValidate(props)
  }

  render() {
    <Field
       validate={this.validate}
    >
  }
}

Or (option 3)

import { memoize } from 'lodash'

const myvalidate = memoize(config => (validateParams) => ( /* some function on validateParams & config */))
const MyComponent = (props) => {
  <Field
    validate={myvalidate(props)}
  />
}

Please note I'm just doing this off the top of my head, and I'm primarily an issues helper and haven't tried this on my personal project yet, so this is just my understanding presently.

[erikras]

I'm more than just "an issues helper". 😄 Two things:

1. Okay, this is silly. The form doesn't need to be reregistered, the form component just needs to be notified of a new validation function.

2. That said, and perhaps I should write a medium post about this...

Creating a new function for a prop in render() is an anti-pattern.

It [almost always] requires that the child rerender itself, when require('This field is required') always generates the exact same function. Memoizing can get you some benefits here, for sure, as would @danielrob's other example of saving them as this.validateUsername-type members.

It would be wonderful if you could do validate={require('Error message')}, because it looks so beautiful, but that's not the way React works at the moment.

Hope that helps.

[gabrielhpugliese]

Thanks for the info!

What if I depend on a prop? What's the correct way to approach in a stateless component? For example:

const { translate } = props;
<Field
  label="Email"
  name="email"
  component={InputField}
  validate={required(translate('Message Override'))}
  placeholder="email"
  type="text"
/>

I really didn't want to couple required function to the translate function.

[gabrielhpugliese]

I also have another example where I have a range and the message should be something like "Choose between 1 and 10" but the range is configurable and dynamic. I'm kinda struggling with that.

[danielrob]

I'd tweak the wording to be super explicit there:

Defining a function in your render for the purpose of passing it as a prop to a child component is an anti-pattern.

BUT

@erikras in the HOF case described in this issue, this doesn't fly:

Suppose I have a field definition which somewhere up the chain get's passed as a prop fieldDef:

{
  type: 'text',
  max: 5,
}

And I want to create a max 5 validator where the max is 5 from fieldDef.

Well, if Field accepted two props validate, validateParams and bound those for you on mount, you could avoid this anti-pattern, but it doesn't, and I'm not sure it should, because I think in our specific case here the 'anti-pattern' is the preferred option because in this case it's not the anti-pattern.

The whole point of this change is that you can change a validator function on a mounted component:

• But if you're never going to do that, then use option 2 which creates your function once, on mount - and never changes again in your render regardless of new props (so it's not even a case of the anti-pattern anyway).
• If you might do that, then option 3 (memoization) or componentWillReceiveProps with props change checking are your only options, but you are forced to follow the anti-pattern. But the only reason it's an anti-pattern is because it causes unnecessary renders, which memoization avoids... so again, not an instance of the anti-pattern.

Otherwise we need to change the Field api to something like:

validateParams - parameters to bind to the validate function [optional]
validate - (...validateParams, value, allValues, props) => error [optional]

And have Field do the binding for you.

Unless I'm somehow mistaken - please enlighten me!

Relevant reading confirming the anti-pattern statement:

Binding parent props to passed function:
https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/jsx-no-bind.md#protips
https://ryanfunduk.com/articles/never-bind-in-render/
https://daveceddia.com/avoid-bind-when-passing-props/ (See Bonus: Passing Arguments Without Bind at the end)

Passing just any on the fly made function in general:
https://medium.com/@esamatti/react-js-pure-render-performance-anti-pattern-fb88c101332f
https://daveceddia.com/avoid-bind-when-passing-props/ (See way #3)

@duro I'm going to humbly apologize and re-open this as it clearly warrants more discussion... but I'll throw the discussion tag on it.

[duro]

I think that @gabrielhpugliese has a good point. If I wanted the error message to change based on props, defining it once outside the render would limit that. That seems like a legit use case.

[danielrob]

@duro, @gabrielhpugliese's case is precisely the same as your case except that 'Message Override' is the result of calling trans('some string'). This will still === every render so that doesn't change the issue. The problem is that require() is an HOF.

[gabrielhpugliese]

But I don't think optimizations should be done for me in this case. There should be an API where I can optimize. This is too obtrusive IMO.

…

On Aug 9, 2017 8:04 AM, "Daniel Robinson" ***@***.***> wrote: @duro <https://github.com/duro>, @gabrielhpugliese <https://github.com/gabrielhpugliese>'s case is precisely the same as your case except that 'Message Override' is the result of calling trans('some string'). That doesn't change the issue. The problem is that require() is an HOC. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3288 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABMd0hEbi_iodXT_00W3fHoIPsxwE-G9ks5sWUvQgaJpZM4OtQfy> .

[danielrob]

@gabrielhpugliese right so tl;dr the new API as it stands means that you must optimise, you can't generate a new function every render - you must either bind during lifecycle methods/instantiation or use memoization if you have a validator HOF.

[Puneetbh]

I have used option #1

const myvalidate = (validateParams) => ( /* some function on validateParams */)
const MyComponent = () => {
  <Field
    validate={myvalidate}
  />
}

But i am not able to pass field name from myvalidate inside validate so i can use it generic for all field validations.
reference #3310

[dmanningnz]

Looks like the cause of this issue is defaultShouldValidate does not check if field level validators have changed. You can see the correct behavior by passing shouldValidate: () => true to reduxForm (Obviously not a great solution).

[nickpalmer]

If you want to use a higher order function then you need to memoize it so that a new function is not created on every call. This is a very common pattern I use for render level action handlers that need to be parameterized.

Using lodash memoize:

const required = _.memoize((msg = 'Default Message') => value => (value ? undefined : msg));

[gabrielhpugliese]

Is this a good blog post to change your minds about this? https://cdb.reacttraining.com/react-inline-functions-and-performance-bdff784f5578

[wmertens]

I agree with @gabrielhpugliese, inline functions are "bad" but they are not "omg the app will explode".

I am hoping that this behavior is reverted to v6.8. If there is a performance issue, inline functions are the first place to look, but that doesn't mean you have to force it on everyone. The operative word is "if".

In my case I tried to add a required keyword on my field wrapper that also adds an asterisk to the label, and I used req = fn => v => v ? (fn ? fn(v) : undefined) : 'required' to wrap a given validate function. Alas, this results in infinite re-renders and crashes the app…

[danielrob]

Hi @erikras, I have been all kinds of burning out on my day job lately, so apologies up front for not being more proactive here, but also this validation issue is all kinds of wrong:

1. First of all the unit tests for Handle changes to Field validate/warn props #3094 were not sufficient - the implemented solution only works when going from no validator to adding a validator, and at no other time (that I can find). See https://codesandbox.io/s/2z42pknypr. This introduces Field level validation must revalidate on component re-register. #3500.
2. The field-level validation API does not cater well for fields (including their validators) being defined by an unknown schema (until the store is updated with it). For example, I could need a maxValueValidator to be have max any number. Before Handle changes to Field validate/warn props #3094, a small use of an anti-pattern allowed getting away with this by going validator={maxValueValidator(max)}. A non anti-pattern ideal solution would be to pass the fieldProps to the validator:

const maxValue = ({ value, allValues, formProps, name, fieldProps: { max } }) =>  (
  if (max && max < value ) {
     return 'maxValueError'
  }
)

This would essentially achieve the same as validator={maxValueValidator(max)}, but without the anti-pattern. There are other solutions, all of which will probably involve half a weeks refactoring when upgrading our project from 6.8.0. For example looking up each schema from the existing formProps.

In short: I'm dubious about what #3094 was actually trying to achieve given that it doesn't apparently completely work, and I agree with the consensus in this discussion that the loss of some sneaky anti-pattern usage is a real pain in the neck though I there are better solutions than just reverting.

[dmanningnz]

@danielrob pre #3094 when a Fields name was updated the new name was registered without the validation functions. #3094 made two changes, including validation functions when the name is updated and re-registering when validation functions changed. Reverting the 're-registering when validation functions change part' may solve some of these issues, but I think another mechanism will be needed to notify the Form component validation needs updating after a change.

[olgeorge]

Any plans on fixing this or reverting to 6.8?
Maybe I'm missing something, but is there any reason to enforce specific usage pattern on users instead of letting them decide and tackle their own performance problems (if they even exist at all, @gabrielhpugliese got a point here)?

[eladlevy]

Hi, migrating to redux-form v7 took me hours because of this change, it was pretty hard to find that passing hoc validation function was the problem.
don't you think this change should be better documented? might as well under Breaking Changes somewhere ?

[danielrob]

I'm closing this in favour of #4017.