teslamotors / informed

A lightweight framework and utility for building powerful forms in React applications
https://teslamotors.github.io/informed
MIT License
950 stars 173 forks source link

What's the logic behind Form serializing values before passing them to onSubmit? #74

Closed JaffParker closed 5 years ago

JaffParker commented 6 years ago

I've been wondering that since the days of react-form. In the FormController component there's following code:

if (this.valid()) {
  if (this.hooks.preSubmit) {
    this.values.rebuild(
      this.hooks.preSubmit(JSON.parse(JSON.stringify(this.state.values))) // this
    );
    this.emit('change', this.state);
    this.emit('update', this.state);
  }
  if (this.hooks.onSubmit) {
    this.hooks.onSubmit(JSON.parse(JSON.stringify(this.state.values))); // this
  }
} else {
  if (this.hooks.onSubmitFailure) {
    this.hooks.onSubmitFailure(
      JSON.parse(JSON.stringify(this.state.errors)), // this
      JSON.parse(JSON.stringify(this.state.asyncErrors)) // and this
    );
  }
}

I marked parts related to my question with comments.

Now why I'm even bothering about this: it's because I try to make sure that string dates don't slip in my application, once they reach the code, they are moment object. However, without any hacks the forms return them as strings, which forces me to either use boilerplate or hacks:

import React from 'react'
import {Form as ReactForm} from 'informed'
import {mapValues} from 'lodash/object'
import moment from 'moment'

const parseValues = values => mapValues(values, str => {
  const date = moment(str, moment.ISO_8601)

  return date.isValid() ? date : str
})

export const Form = ({children, onSubmit, ...props}) =>
  <ReactForm {...props} onSubmit={values => onSubmit(parseValues(values))} preSubmit={parseValues} preValidate={parseValues}>
    {children}
  </ReactForm>

My date fields actually return moment objects, so this is just additional work that backlashes in terms of performance... So my question is whether it's necessary to serialize fields and will anything be majorly broken, if it's removed?

joepuzzo commented 6 years ago

So i try to keep everything generic types in the state for simplicitys sake in order to make things like render optimization easy. Also the stringifying and parsing is another way to return immutable objects. Somthing i would like to do more

JaffParker commented 6 years ago

Would it be possible to make it optional or would it potentially break things? My heart breaks every time I realize that I parse literally every value in every form to see whether it's a moment or not, it would be much easier if they could just stay moments...

ulybu commented 6 years ago

Hey Joe,

In addition of Moments , it is also an issue with immutables (facebook's immutable collection). I have initial values comming as immutables and they're supposed to be fed to custom fields (using HOC asField) but those custom fields always receive plain JS objects.

So I completely understand the value of deep cloning objects out of the box so the user doesn't have to worry about mutability. Nevertheless, having it as an opt-in option and therefore let the user handle the immutability in the handlers would extend the scope of use of Informed for all developers who use non plain-JS format

joepuzzo commented 5 years ago

I have updated the lib to no longer do this.