'model string' is always being considered as Property Accessor, initialized array is being converted to an object with first property as an array

sthuku commented 7 years ago

The Problem

I have an object which has bunch of properties some of the properties are initialized as array of objects,

const initialObj = { property1: '', property2: '', property3: [ {val: '' } ] }

I have multi-select, whenever the user select/check items, they should be saved in an object of an array. That means the selected values should be saved here obj.property3[${index}].val. I want to have the following object in my redux store

{ property1: '', property2: '', property3: [ {val: 'some value' }, {val: 'some value' }, {val: 'some value' }, {val: 'some value' }, so on ] }

This is my model string for that multi-select obj.property3[${index}].val where 'index' is the 'selectedList.length' (I am getting the list that already exists or will exist from the multi-select) and initially, the index is set to 0.

//index is from redux store function mapStateToProps(state) { return { index: state.obj.property3.length } }

The result is

{ property1: '', property2: '', property3: { 0:['some value' , 'some value', 'some value', 'some value', so on ] } }

I am expecting an array of objects containing the selected values from multi-select but instead, the result is an object with one key '0' which is set to an array of values (from multi-select).

So when I do do this obj.property3[${index}].val, the index value in square brackets is being considered as a property accessor instead of an array index.

I have tried "track", it is not working in this case, if you have something like {id:303, value: 'somevalue'} you can track but how do you track an array index.

If I try custom actions with "actions.change", the multi-select is not being populated with the data from the backend (HTTP GET) I want to avoid modifying the form data for GET/POST/PUT requests.

Steps to Reproduce

Expected Behavior

Actual Behavior

Reproducible Code Example

(please fork from this CodePen template)

davidkpiano commented 7 years ago

Can you please make a reproducible example using the CodePen template? It's the quickest way for me to diagnose/fix these issues.

creatorkuang commented 7 years ago

I have the same issue. I have update the codepan template and you could check it. https://codepen.io/anon/pen/rwbPYo

davidkpiano commented 7 years ago

If you initialize the array (that way letting RRF know it's an array), it works fine:

Otherwise, we're making assumptions - if the property doesn't exist, how will we know whether it's meant to be an array or an object?

foo.bar['baz'].name: definitely in an object
foo.bar[0].name: in an array
foo.bar.0.name: in an object... or an array?
foo.bar[567].name: probably in an array
foo.bar[3].name when foo.bar is already an object: in an object

There's a little ambiguity, but I might be able to be convinced that [<positive integer>] in an undefined property value should be an array. Right now, it defaults to objects.

creatorkuang commented 7 years ago

I agree with you that we could not identify whether it's array or object with this dash model. In some case, it's hard to set the default value . For my case , we allow user to drag and drop to define the form and they don't need to set up their initialState. So, I think what you said might be good.
if the model is[<positive integer>],you identified it as an array, and if user want to use object, use .<positive integer>.

creatorkuang commented 7 years ago

@davidkpiano could you point me out where is right place to enhance this, I could spent sometime to do this enhancement.

davidkpiano commented 7 years ago

If you want to try to do this, it's lines 9 - 11 of model-reducer.js:

function icepickSet(state, path, value) {
  return i.setIn(state, path, value);
}

Currently, icepick.setIn() will always recursively create objects for each key in path. I think the solution will have to be that we implement a custom setIn() (if you look at the icepick code for assocIn you'll see it's very terse).

However, there's already a custom utils/assoc-in.js, so we can just reuse that. We will have to modify the logic so that instead of defaulting to creating an empty object for each key (in line 34 of assoc-in.js), it does the following:

Coerce the key to a number
Check if that coerced key is indeed a number
Check that the coerced key is an integer (Math.floor(n) === n)
Check that the coerced key is >= 0

If those are all true, create an empty array instead of an empty object.

And then we just have to do that for the Immutable.JS implementation too 😅

Also, unit tests.

Hope that explains it all!

creatorkuang commented 7 years ago

@davidkpiano I have done what you propose and it throw me out with this error: which is point to reducers/form-actions-reducer line 97 const { intents } = fieldState;. The code in utils/assoc-in.js as below and i have already test it. Please point me out what i should do next?

import identity from './identity';

function objClone(obj,isArray) {
  const keys = Object.keys(obj);
  const length = keys.length;
  const result = isArray?[]:{};
  let index = 0;
  let key;

  for (; index < length; index += 1) {
    key = keys[index];
    result[key] = obj[key];
  }
  return result;
}

export function assoc(state, key, value) {
  const isArray = Object.prototype.toString.call(state) === '[object Array]'
  const newState = objClone(state,isArray);

  newState[key] = value;

  return newState;
}

export default function assocIn(state, path, value, fn = identity) {
  if (!path.length) return value;

  const key0 = path[0];
  // try to fix #863

  if (path.length === 1) {
    return fn(assoc(state, key0, value));
  }
  let subVal = {};
  if(state[key0]!==undefined) {
    subVal = state[key0]
  } else {
    const key1 = path[1];
    if(key1 && !isNaN(key1) &&  Math.floor(key1)===parseFloat(key1)) {
      // check path .[Number]. or .Number.
      const valPath = value.model;
      if(valPath!==undefined) {
        const key1Idx = valPath.indexOf(key1);
        const keyGap = key1.toString().length;
        if(valPath.substring(key1Idx-1,key1Idx)==='[' && valPath.substring(key1Idx+keyGap,key1Idx+keyGap+1)===']') {
          subVal = [];
        }
      }
    }
  }

  return fn(assoc(state, key0, assocIn(subVal, path.slice(1), value, fn)));
}

davidkpiano / react-redux-form