Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added nested values proposal prototype (#202) #207

Merged
merged 13 commits into from Nov 27, 2017

Conversation

klis87
Copy link
Contributor

@klis87 klis87 commented Oct 12, 2017

This is my quick suggestion to solve issue #202 . It is far from perfect (I don't use Typescript so I neglected types), also, I couldn't make lodash/fp working with Typescript, so I needed to use mutable _.set, and sadly I used it with deepClone. Of course this would need to be refactored, but this is just a prototype.

If you like this idea, we could work on it more, add missing tests etc.

The good news is, that all tests are passing so it is backward compatible change :)

@jaredpalmer
Copy link
Owner

jaredpalmer commented Oct 12, 2017

Here was my version ( very simiiliar). Mine uses developit's dlv utility, which does _.get in 128 Bytes. (see below)

My concern with keeping this in <Field> and not in <Formik> is that setFieldValue would operate differently depending on where you call it. This is a code smell IMHO.

import React from 'react';
import PropTypes from 'prop-types';
import dlv from 'dlv';

// inspired by @developit's linkState
function setDeep(path, value, obj) {
  let res = {};
  let i = 0;
  for (; i < path.length - 1; i++) {
    res = res[path[i]] || (res[path[i]] = (!i && obj[path[i]]) || {});
  }
  res[path[i]] = value;
  return res;
}

class Field extends React.Component {
  static contextTypes = {
    formik: PropTypes.object,
  };

  handleChange = e => this.setValue(this.props.name, e.target.value);

  handleBlur = e => this.setTouch(this.props.name, true);

  setValue = (key, value) => {
    let path = key.split('.');
    if (path.length > 1) {
      this.context.formik.setFieldValue(
        path[0],
        setDeep(path, value, this.context.formik.values)
      );
    } else {
      this.context.formik.setFieldValue(key, value);
    }
  };

  setTouch = (key, value) => {
    let path = key.split('.');
    if (path.length > 1) {
      this.context.formik.setFieldTouched(
        path[0],
        setDeep(path, value, this.context.formik.touched)
      );
    } else {
      this.context.formik.setFieldTouched(key, value);
    }
  };

  render() {
    const { component = 'input', name, ...props } = this.props;
    const field = {
      value:
        props.type === 'radio' || props.type === 'checkbox'
          ? props.value
          : dlv(this.context.formik.values, name),
      name,
      onChange: this.handleChange,
      onBlur: this.handleBlur,
    };
    const bag =
      typeof component === 'string'
        ? field
        : {
            field,
            form: {
              ...this.context.formik, 
              handleChange: this.handleChange,
              handleBlur: this.handleBlur,
              setFieldValue: this.setValue, 
              setFieldTouched: this.setTouch 
            }
          };
    return React.createElement(component, {
      ...props,
      ...bag,
    });
  }
}

@klis87
Copy link
Contributor Author

klis87 commented Oct 12, 2017

But I guess your approach is different, right? In my pull request setFieldValue itself is responsible for setting nested value, so Field component doesn't need to now about implementation detail. It is working in all cases: setFieldValue('flatName', value), setFieldValue('value.nested.nested2', value) etc.

I agree that setters logic shouldn't be part of Field component, this should belong to Formik component, otherwise it won't be consistent. Not to mention that this won't cover all cases - like setting errors or setting dirty state for all inputs (in touchAllFields function). Also, people might want to implement own versions of Field component, or use some setter callback outside of it - in my opinion Field should be treated like helper function/component, and nested path functionality should be really in the core of this library.

So to sum up, would you consider adding nested objects handling in this library? If yes, what are steps required to merge it? I can think of the following:

  • adding missing tests to prove that setters understand path concept
  • making sure that it would work with arrays
  • optimizing setters performance
  • updating docs of course

Btw, this form library is really perfect, I think that nested object handling is the only thing missing 👍

package.json Outdated
@@ -34,6 +34,7 @@
"gen-docs": "all-contributors generate && doctoc README.md"
},
"dependencies": {
"lodash": "4.17.4",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use dlv as described. no need to import all of lodash 😄 !

package.json Outdated
@@ -45,6 +46,7 @@
"devDependencies": {
"@types/enzyme": "2.8.4",
"@types/jest": "20.0.6",
"@types/lodash": "4.14.77",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove

@jaredpalmer
Copy link
Owner

jaredpalmer commented Oct 13, 2017

To make things easier. Remove dlv and inline it along with setDeep:

// utils.ts

/** 
 * @private Deeply get a value from an object via it's dot path. 
 * See https://github.com/developit/dlv/blob/master/index.js
 */
export function dlv(obj: any, key: string | string[], def: any, p: number = 0) {
  key = (key as string).split ? (key as string).split('.') : key;
  while (obj && p < key.length) {
    obj = obj[key[p++]];
  }
  return obj === undefined ? def : obj;
}

/** 
 * @private Deeply set a value from in object via it's dot path. 
 * See https://github.com/developit/linkstate
 */
export function setDeep(obj: any, key: string | string[], value: any) {
  key = (key as string).split ? (key as string).split('.') : key;
  let res: any = {};
  let i = 0;
  for (; i < key.length - 1; i++) {
    res = res[key[i]] || (res[key[i]] = (!i && obj[key[i]]) || {});
  }
  res[key[i]] = value;
  return res;
}

@klis87
Copy link
Contributor Author

klis87 commented Oct 13, 2017

I removed lodash dependency. For deepSet I used your suggestion, but I needed to adjust it slightly as it should return the whole object, not only nested part of it. Also, path can contain brackets to set array items, so list.0 and list[0] is the same - I needed to do it because yupError.inner paths are in this format. I admit it is a little messy and I think the best would be to cherry pick lodash/fp/set.

I tested nested values including array and now sth like <Field name="list.0" /> is possible - there won't be any need to flatten arrays anymore - its is working with values, errors and touched.

@jaredpalmer
Copy link
Owner

Problem I see is that we are fully cloning and replacing all of values 2x on each keystroke now. I need to run this on my benchmarking suite to see perf implications.

@klis87
Copy link
Contributor Author

klis87 commented Oct 13, 2017

Regarding your comment, like I mentioned above, please see my adjusted setDeep. Your version has 2 problems:

  • it expects path as array and I believe splitting path like 'x.y' into ['x', 'y'] should be really done inside, otherwise splitting will need to be repeated before every call of this function
  • it returns incorrect value for nested paths - only last portion of object

Regarding perf implications, I am waiting for the results

@klis87
Copy link
Contributor Author

klis87 commented Oct 13, 2017

I believe it should be fine, as we do only shallow clone by { ...obj, ...res } in setDeep

Not to mention that actually values were already cloned before this PR, for example:

values: {
   ...prevState.values as object,
  [field]: val,
}

@klis87
Copy link
Contributor Author

klis87 commented Oct 16, 2017

I inlined dlv. Also, I modified slightly our seetDeep method - I replaced (resVal[pathArray[i]] = (!i && obj[pathArray[i]]) || {}); with (resVal[pathArray[i]] = (!i && { ...obj[pathArray[i]] }) || {}); to avoid obj mutations in some cases. Also, I added tests for seetDeep to be sure it returns correctly updated object and doesn't mutate input object.

Btw, what is this !i for, tests are passing with or without it, is there any case I am missing which make it necessary?

Also, we have small problem with arrays and yup with following schema:

const schema = yup.object().shape({
  list: yup.array().of(yup.string().required())
});

The problem is that our implementation won't save values as array but as object like { 0: 'a', 1: 'b' }, usually it wouldn't be any problem but it is for errors as yup validator will complain list is not array and it wont work. We have 2 solutions, either adjust our deepSet once more, or maybe lets just use lodash/fp/set? It gives us exactly what we want, and you already use lodash.isequal so there is no shame in it :D

@klis87
Copy link
Contributor Author

klis87 commented Oct 16, 2017

I troubled me too much and now our setDeep supports arrays, yup doesn't complain anymore.

@gangstaJS
Copy link

When do you merge this pull request? I need this feature)

@gangstaJS
Copy link

gangstaJS commented Oct 18, 2017

Looks like enableReinitialize do not works correct with type of field this.props.setFieldValue("phone[0].value", text). I will try create code example for this case

@klis87
Copy link
Contributor Author

klis87 commented Oct 19, 2017

@gangstaJS What is not working, maybe your value is not correctly displayed in Field component? Then try phone.0.value in name prop. dlv getter doesn't anticipate brackets in path, only dots. deepSet is compatible with brackets, because yup uses brackets for error paths. In my opinion, we should add brackets support do dlv as well - this is how _.get is working and what people generally expect.

@gangstaJS
Copy link

gangstaJS commented Oct 19, 2017

@klis87 As I see it replaced to an empty string var pathArray = path.replace(/\]/g, '').split(/\.|\[/); at setDeep. So [] does not matter
I am using your branch with this PR.

Looks like deep elements like arrays etc. are lost reference after setFieldValue http://take.ms/ys9vk or something else

@klis87
Copy link
Contributor Author

klis87 commented Oct 19, 2017

It is probably because you are pushing and removing array items. I tested arrays and it is working for me, but my array had a fixed length. I didn't try to add new items or remove them. I will play with this use case today and I will give you my results. If necessary, I will try to fix it.

Another tip - if you have initialValues={{ list: ['a', 'b'] }}, you cannot just use Field name="list.2" /> because it simply doesn't exist yet. Otherwise you would have FormControl is changing an uncontrolled input of type text to be controlled. Input elements should not switch from uncontrolled to controlled (or vice versa). warning, maybe this is what you have?

@gangstaJS
Copy link

gangstaJS commented Oct 19, 2017

@klis87 I have hot fixed problem. See my bullshit code :)

if (this.props.enableReinitialize && !index$5(nextProps.initialValues, this.state.values)) {
            this.initialValues = __assign({}, this.state.values, nextProps.initialValues);
            this.resetForm(__assign({}, this.state.values, nextProps.initialValues));
        }

there is 3275 line in trunspiled not minifed code

I haven't any warnings with the issue, regarding yours question above

@klis87
Copy link
Contributor Author

klis87 commented Oct 19, 2017

So it was bug in your app? Or we need to add something to Formik to handle this case?

@gangstaJS
Copy link

gangstaJS commented Oct 19, 2017

I think this is a bug of formik. I have fixed it at the source code of formik

@klis87
Copy link
Contributor Author

klis87 commented Oct 19, 2017

Ok, so again, I will try to prepare some demo for this use case later today.

@gangstaJS
Copy link

I am not very familiar with TS so I just edit complied code, sorry

@klis87
Copy link
Contributor Author

klis87 commented Oct 19, 2017

No problem. I feel that this is not formik issue anyway, but they way you add items to your array. The demo will show us what to do

@gangstaJS
Copy link

gangstaJS commented Oct 19, 2017

i do it with redux

case ADD_NEW_PHONE_ROW:
      current = Object.assign({}, state.current);
      current.phone.push({value: '', type: 'work'});

      return Object.assign({}, state, {
        current,
});

@jaredpalmer jaredpalmer merged commit 3a8e620 into jaredpalmer:master Nov 27, 2017
@goodmind
Copy link

goodmind commented Nov 30, 2017

But you can't write types for something like this api, no?

<Field name={`list.${i}`} />

@jaredpalmer
Copy link
Owner

dlv is the get method. I think we might need to adjust it

@klis87
Copy link
Contributor Author

klis87 commented Nov 30, 2017

@goodmind it should work. I used current version of Formik with dynamic arrays and it was fine (as long as you have an item with a given index in your list. Here you can see a demo I used:

const schema = yup.object().shape({
  list: yup.array().of(yup.string().required())
});

<Formik
  initialValues={{ list: ['a', 'b'] }}
  validationSchema={schema}
  onSubmit={(values) => {
    console.log(values);
  }}
>
  {({ values, setFieldValue }) => (
    <Form>
      {values.list.map((v, i) => (
        <div key={i}>
          <Field name={`list.${i}`} />
          <button
            type="button"
            onClick={() => {
              const n = [...values.list];
              n.splice(i, 1);
              setFieldValue('list', n);
              // we should update touched and potentially status state here
            }}
          >
            Remove {i}
          </button>
        </div>
      ))}
      <button
        type="button"
        onClick={() => {
          setFieldValue('list', [...values.list, '']);
        }}
      >
        add
      </button>
      <button>Save</button>
    </Form>
  )}
</Formik>

@jaredpalmer
Copy link
Owner

jaredpalmer commented Nov 30, 2017

@goodmind to get an index shouldn't it be: <Field name={list.thing[${i}]} /> ?

@klis87
Copy link
Contributor Author

klis87 commented Nov 30, 2017

@jaredpalmer I think dlv doesnt respect brackets but I might be wrong. But it definitely works with dots.

@jaredpalmer
Copy link
Owner

@klis87 thanks for this PR btw. I think we should add bracket support to dlv?

@klis87
Copy link
Contributor Author

klis87 commented Nov 30, 2017

@jaredpalmer Thanks for considering and merging this :) I think we should add this. Especially I needed to add bracket support for our nested setter, because yup creates paths for array items using brackets, not dots. Also, lodash.get supports brackets, and this is what people generally would expect working anyway.

@klis87
Copy link
Contributor Author

klis87 commented Nov 30, 2017

Btw, would you consider adding sth like this from redux form ? Because without it is is hard to add/remove items from array - you need to do it like i did in my snippet, instead we could expose alternative actions to setFieldValue, like push, pop, insert etc.

@jaredpalmer
Copy link
Owner

@klis87 I agree and think lodash's path syntax is the goal. As for array helpers, my plan is see how far I can get with a <FieldArray render={({ sort, insert, pop, push, delete }) => }> component to start.

@jaredpalmer
Copy link
Owner

@klis Let's make a new issue for that as this has been merged

@goodmind
Copy link

I meant TypeScript types

@jaredpalmer
Copy link
Owner

jaredpalmer commented Nov 30, 2017

Sorry, misread that. It's still a string, but typing Field properly is basically impossible anyways because there is no such thing as a JSX generic. So we punted as just have good old PropTypes .

@zachlatta
Copy link

zachlatta commented Dec 21, 2017

Hi, I've spent the past few hours trying to figure out how to use the changes from this pull request. I can't find any documentation on using nested values.

Can someone provide an example of how to create a form with nested values using the changes made here?

@bs1180
Copy link

bs1180 commented Dec 21, 2017

@zachlatta Have you installed the next branch? That caught me out - you need to run yarn add formik@next, and then it should work out of the box. The example is also updated.

@zachlatta
Copy link

zachlatta commented Dec 22, 2017 via email

@mrchief
Copy link

mrchief commented May 16, 2018

I tried @next but doing

setFieldValue(`${bag}.${id}`, { id, name }, false)

causes Chrome tab to crash.

@mrchief
Copy link

mrchief commented May 16, 2018

Ok, this is not a Formik issue. My id is a number and it seems redux chokes on that. Without dev tools open, it works but is sluggish. With devtools open, it goes out of memory. Happens with redux-form too so it's definitely not Formik.

@mrchief
Copy link

mrchief commented May 17, 2018

Nested numeric ids seem to be problem but it's not dev tools. It suffers without it too. I opened up redux-form/redux-form#4034 but the exact same thing happens with Formik as well. Maybe the same or similar causes exists here too (the code to set nested objects paths is probably the likely cause) .

Might be a good idea to check it out...

@Skand17
Copy link

Skand17 commented Jul 21, 2020

    return React.createElement(component, {

Thanks @jaredpalmer . This look good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet