Question: Should removing a model entry remove the associated field?

[mdgbayly]

Calls to

action.remove('myModel', index)

....don't remove fields from the form's fields object associated with that model entry.

Is this by design? It has the side effect that if you have an error associated with an array field, and you delete that field, the error shifts to the array element that was below.

Also, adding items back into the array can cause them to inherit errors from previously deleted array entries.

I'm not using any field validators so maybe that is related? Maybe I should be resetting field validity when I remove things myself? But don't see a way to remove things from the fields object?

It also means you can lose errors too.

e.g. you have a list of 3 fields and item 3 has an error and item 2 doesn't. You delete item 2 and now item 3 inherits item 2's valid state.

[mdgbayly]

Hmm, I guess it's a bit more complicated? Because the fields object has named properties? So if you remove something you have to rename all the properties that come after it to take into account the changed indexes?

[mdgbayly]

Hmm - maybe hold that thought - i am seeing some very strange behaviour when removing - maybe I have screwed something up.

[mdgbayly]

OK - my weird deletion issues were a different problem. So yes, I think my question still stands regarding deleting fields when you remove a model entry?

[davidkpiano]

That's probably a bug. I just need to handle the .remove action in the form reducer as well. I will fix this.

[davidkpiano]

Fixed in 0.9.3!

[mdgbayly]

Thanks, this is definitely better, but I don't think it fully addresses the issues.

e.g. if you have an array of 3 items and the third one is invalid you'll have field state like:

item.0: {valid: true}
item.1: {valid: true}
item.2: {valid:false}

If you now delete the second item you end up with:

item.0: {valid: true}
item.1: {valid: true}

whereas I would expect:

item.0: {valid: true}
item.1: {valid:false}

I think the fix you added is removing fields that no-longer have an entry in the model, but it isn't retaining field state. I think part of the difficulty with implementing something like this is that the model reducer funnels most changes through the CHANGE action. Whilst this is great for simplicity (in fact I love the simplicity of the model reducer!) I think it's losing information.

I wonder whether the action creators should be passing the 'change based' actions on explicitly rather than defining a new model state in the creator and pushing all changes through CHANGE. My impression is it wouldn't be a big refactoring to move some of the icepick manipulations for things like REMOVE to the model reducer. Then the form reducer would have more raw information about the change on which to correctly adjust field states.

I face a similar issue with the MOVE action I implemented. In our app we can move items that have errors associated with them and ideally we'd like the errors to be retained. When I get a few spare cycles I was going to have a play to see if I can come up with some ideas.

Cheers

[davidkpiano]

Unfortunately it's not as easy as having an ad-hoc { type: actionTypes.REMOVE } action. I'm thinking of a better, more robust solution, similar to Angular 1's track by (and Angular 2's ngForTrackBy.

Why? Consider a model that has the elements [0, 1, 2, 3]. If you move, filter, or remove items and RRF handles the actions in a way that maintains field references based on item value, you'll get your expected result.

But what if you do an actions.map action that increments each item? Then you'll have [1, 2, 3, 4], which ambiguously looks like some items have shifted, instead of changed, to RRF.

So let's consider a syntax similar to this:

import { Field, track } from 'react-redux-form';

// consider a state like:
const items = [
  { id: 100, foo: 'bar' },
  { id: 200, foo: 'baz' },
  { id: 300, foo: 'etc' }
];

// ...

function getID(item) {
  return item.id;
}

// in render() return:
{ myModel.items.map((item) =>
<Field model="myModel.items[]"
  key={item.id}
  trackBy={getID}>
  <input type="text" />
</Field>
}

Is this too weird? Let me know your thoughts.

[davidkpiano]

^ Oh, forgot to mention, this would mean that there would be a new .key property on the field object, so you'd have something like this:

myModel.fields = {
  'items.0': { key: 100, ... },
  'items.1': { key: 200, ... },
  'items.2': { key: 300, ... },
}

And probably a helper function or a third parameter to the getField() helper function:

{ getField(myModelForm, 'items', 200).valid
  ? <div>Valid!</div>
  : <div>Not valid.</div>
}

[chrisblossom]

Maybe I am completely misunderstanding what is going on/what the issue is, but wouldn't it be a lot better/sane to stop using forms inside arrays and store them in a plain object with an array pointer for order, similar to gaearon/normalizr.

const redux = {
  order: [1, 2, 3, 'foo'], // <-- Keep track of order here
  forms: {
    1: {
      // form 1
    },
    2: {
      // form 2
    },
    3: {
      // form 3
    },
    foo: {
      // form foo
    },
  },
};

console.log(redux.forms[redux.order[3]]); // foo

[mdgbayly]

@chrisblossom I'm not sure you're talking about exactly the same thing but your comment has made me stop and pause. Our issue is that we have an array of data in a single form? Your example looks like it's referring to multiple forms in an array or maybe I'm just misunderstanding what you mean by a 'form 1' etc.

This is a simplified view but our app contains arrays in it's state like:

state: {
    items: [
        {
             identifier: 'answer1',
             text: 'Green'
        },
        {
             identifier: 'answer2',
             text: 'Red'
        }
    ]
}

We can associate errors with the attributes in that state so if the state value 'Green' is invalid then we might have a field entry like:

state: {
    answerForm: {
        valid: true,
        ...
        fields: {
            items.0.text: {
                valid: false
                ...
            },
            items.1.text: {
                valid: true
                ...
            }
        }
    }
}

The issue I'm dealing with is that if the user now deletes 'answer1'/'green', then answer2 now becomes the first item in the array, and unless we fix up the field state, it appears like answer2 is now invalid. So at one level you need to 're-order/rewrite' all the field states to match the array state because the '.0.' and '.1.' in the field names represent array offsets.

The reason you're making me stop and pause is because I'm realizing that dealing with arrays seems to be tricky. I've had similar discussion with @davidkpiano about issues relating to managing arrays in another area of our app and using normalizr was one suggestion he had made.

We're using arrays currently because that's the shape of the data that comes over the API we call and they seemed to be the simplest approach and to be supported by RRF.

But I can see now how they make managing field state complex and turning the '.0.' in the field name into a fixed identifier rather than an array offset would make things a lot less fragile.

Will give it some more thought.

[mdgbayly]

@davidkpiano

Why? Consider a model that has the elements [0, 1, 2, 3]. If you move, filter, or remove items and RRF handles the actions in a way that maintains field references based on item value, you'll get your expected result.

I need to think about your suggestions some more but I'm not sure I was suggesting to maintain field references based on value? I was thinking that if field references represent array offsets in the state and you change the state of those arrays, then you need to pass the modified array offsets somehow to the field reducer in order for it to correctly update the field state.

But it is cumbersome...

If you have an array of fields:

item.0.text: {}
item.1.text:{}
item.2.text:{}

and you remove the state associated with item.0 then you have to rename all the fields above it to fix the array offsets.

So maybe you are suggesting using the trackby keys as an alternative to array offsets?

Or as @chrisblossom was suggesting maybe, steer clear of arrays in your state as they are trouble? Instead base everything around objects?

So will think about it some more. In our model state that I showed in the previous post, we could potentially normalize around the item.identifier. I've been reluctant to do that up until now as we don't control the namespace of that identifier and wasn't sure if it might contain values that are invalid javascript property names, But reading some posts on that it seems like there are not many things that are invalid property names if you use bracket syntax?

Or alternatively we could introduce a level of indirection as maybe suggested by @chrisblossom so that we can still deal with array data as objects.

[davidkpiano]

So maybe you are suggesting using the trackby keys as an alternative to array offsets?

Yes, and one good reason for this is that things will get really complicated if we keep track of array offsets rather than keys, and tracking by keys is more robust. Consider this:

• Data from API: [1, 2, 3, 4]
• Client data after shuffling: [2, 3, 4, 1]
• Client maintains array offsets
• Client saves to API
• Hard refresh page and grab data from API again: [2, 3, 4, 1]
• Array offset references are lost.

It's better to reference by a unique ID.

Or as @chrisblossom was suggesting maybe, steer clear of arrays in your state as they are trouble? Instead base everything around objects?

You can do this; RRF is not opinionated and will work perfectly fine if you use something like normalizr.

[davidkpiano]

@mdgbayly I'm introducing an enhancement in the next minor version (currently on the master branch) that solves this succinctly, using the track(model, predicate) function, where the predicate can be a:

• function that returns true for the first valid field, or...
• a handy Lodash Array, Object, String shorthand: https://lodash.com/docs#findKey

import { Field, track } from 'react-redux-form';

// ...
<Field model={ track('team.users[].name', {id: 1234}) }>
  <input type="text" />
</Field>

^ The above code will update the user.name in the team.users array whose id == 1234. This will work as expected even if the array is shuffled, manipulated, mapped, filtered, deleted, etc. :smile:

Coming to 0.10.0!!

[mdgbayly]

cool - thanks - i actually took some of the advice from Chris and yourself regarding how to model things and we've modified our API format to normalize the data in some areas to make it easier to deal with. But this enhancement looks like it will be really cool to have too.