Setting values via path array?

[lewisl9029]

Hi,

I saw in the readme that you can create a cursor by passing in a path array:

var colorsCursor = tree.select(['palette', 'colors']);

So for some reason I ended up assuming that you can also set values by passing in a path array like so:

tree.set(['palette', 'colors'], 'blue');

However this doesn't seem to be the case:

tree.set(['palette', 'colors'], 'blue');
tree.commit();
tree.get();
//Object {palette,colors: "blue"}

Calling set with a path array actually leads to Baobab setting the value at the path of a comma-joined string of the path array, which is quite odd.

The workaround I'm using right now is calling select with a path array and then edit to change the value, but I feel being able to pass in a path array to set would be a more intuitive way to do this.

Thoughts?

Thank you for making this amazing library by the way! =)

[Yomguithereal]

Hello @lewisl9029, I was thinking about this polymorphism when creating the library and found it a bit weird. I therefore waited before implementing it and now, here we are.

Anyway, the fact that setting the value with an array as first arguments ends up setting a comma-separated key is clearly a bug that I should fix.

I could add however, as you suggest, the following polymorphisms in a further release:

tree.set(['palette', 'color'], 'blue');

// The second one seems clunky however
tree.set('palette', 'color', 'blue');

[marbemac]

I too ran into this. Say I have a state tree like this:

var stateTree = new Baobab({
  models: {
    users: {}
  }  
});

I'm using a map instead of an array of users because it's easier to lookup and set this way. So, whenever I need to update a user, I need to do something like this:

var update = {models: {users: {}};
update.models.users[userId] = {$set: newUserData};

stateTree.update(update);

Would be awesome if I could instead do this:

stateTree.set(['models', 'users', userId], newUserData);

[Yomguithereal]

Ok, I will therefore plan to add this polymorphism. What about the second one:

tree.set('palette', 'color', 'blue');

Dangerous (probably yes)?

[Yomguithereal]

As a side question, do you think this polymorphism could be valuable to some other methods like push or unshift, for instance?

[marbemac]

Awesome!

Hmm I don't like the second syntax for two reasons.

1. It's not immediately obvious what is being set, where.
2. Not as flexible as the first option, because you can't build the path dynamically.

What would be super cool is if set built the path if it didn't exist. So for example:

var stateTree = new Baobab({
  models: {}
});

var userData = {id: '123', name: 'Marc'};
stateTree.set(['models', 'users', userData.id], userData);

// stateTree is now {models: {users: {123: {id: '123', name: 'Marc'}}}}

Would definitely be valuable with other methods like push and unshift. Especially if it built paths as described above. So:

var stateTree = new Baobab({
  models: {}
});

var userData = {id: '123', name: 'Marc'};
stateTree.push(['models', 'users'], userData);

// stateTree is now {models: {users: [{id: '123', name: 'Marc'}]}}
// this was a push into the models.users path, so baobab made the users property
// an array, instead of a object (which is what it did in the .set example above)

[Yomguithereal]

Set would indeed build the path if this one does not already exist. It already works thusly for the update specifications or if you edit a value using a cursor on a currently inexistant path.

However, I am not that confident about the push thing. I would prefer the user to explicitly set the node as an array before he can push to it. But give me some time to think about it.

[lewisl9029]

I personally only plan on using the tree.set(pathArray, object) signature for precisely the reasons @marbemac outlined above. But I think the other version should probably exist if only for the sake of maintaining consistency between the set and select APIs' path input.

Also, @marbemac, I think the workaround I'm using right now might be of some use to you, since it's a bit less verbose than your snippet with .update():

stateTree.select(['models', 'users', userId]).edit(newUserData);

As an API I find this more elegant, but I'm not quite sure of the performance implications of this approach since we're also creating a new cursor with each edit. Though I feel it shouldn't be too much overhead since no listeners are being registered here. Being able to call set directly using a path array would definitely be a much better alternative.

To handle the push use case @marbemac mentioned above, I think simply using set with an array as value would be more explicit and less prone to errors and confusion for users, and less unnecessary implementation complexity:

var stateTree = new Baobab({
  models: {}
});

var userData = {id: '123', name: 'Marc'};
stateTree.set(['models', 'users'], [userData]);
// stateTree is now {models: {users: [{id: '123', name: 'Marc'}]}}

But please enlighten me if I'm missing some other use case that can't be achieved this way.

[christianalfoni]

Hi guys, just giving my two cents.

I think it is important for consistency that a selector is a selector. Currently in Baobab the select() method does the selecting. Allowing .set() to also have a path selector as a first argument breaks that rule. That said, I definitely see it would be nice to have :-)

So I have two other suggestions, just to sugar the discussion.

Remove set() It could be an idea to just remove set() as that would keep the API consistent and easier to use, one less method to learn. Anything you can do with set() you can do with edit(), but as @lewisl9029 pointed out, you return the cursor of the property edited, not the object where you set the property. That in itself I do not think is an issue, but maybe there are performance issues? @Yomguithereal?

Remove select() Yeah, I am crazy ;-) Maybe the initial API was not the perfect angle for Baobab as we see these new scenarios appear. So instead of defining the path as an API method you could have that as the first argument of all methods:

tree.set(['imodels', id], { foo: 'bar'});
tree.push(['items'], 'foo');
tree.edit(['level1', 'level2'], []);
tree.merge('config', {});

What we see here is that set and edit does the same thing, but with this API I think it would make more sense to remove edit and keep set.

I think it is important to strive for consistency and simplicity. Adding a special path selector on set() by definition adds complexity, though again, I totally understand why you want it. I want it too ;-)

[marbemac]

@christianalfoni The old syntax, .set(key, value), was already a "selector" of sorts. It was just a crippled selector, in that you could only select by flat key. If what you're looking for is a strict separation, I think we'd need to do something like:

tree.select(['users', userId]).set(newUserData);

Basically, select returns a cursor, set acts on a cursor and overwrites whatever the cursor is pointing to. In this scenario set does no "selecting/looking up of data". I understand the desire for consistency and simplicity, but I think the path selector on set is just too useful to pass up.

I agree that adding a path selector on all of the methods (set, push, merge, remove, etc) would make sense, and keep everything consistent. Not to mention it'd be super useful :).

[christianalfoni]

Yeah, I think the version of set() and edit() are now overlapping and causing confusion. There really is no need for both of them. As you point out:

tree.select(['users', userId]).set(newUserData);

Would indeed just replace edit() and I think that is a good thing. One less method and more consistency :-)

Changing to tree.set(path, value), tree.push(path, value) etc., after some thought, would not work that well. You still need some way to just select a cursor, without mutating it. So yeah...

@Yomguithereal , if people agree, I would suggest removing the edit() method on v.1.0.0 and only use set() the way @marbemac showed an example of here. One less method and more consistency :-) Unless there is something we are not considering here of course.

[Yomguithereal]

The issue here @christianalfoni is that select aims at returning a cursor and will create a cursor in the process. Even if the lib's cursor are lazy and won't create listeners lightly, I find it a bit overkill sometimes to use a select just to edit data and prefer a light polymorphism on the set method. tree.set([path], value) does not create any cursor in the process. But I guess this is a question of taste.

Furthermore, there is a conceptual difference between set and edit. set is very connotative in the js world and you usually set a key to refer to a precise value. Using set instead of edit would break this implicit meaning. Furthermore, I would find it very tedious to have to select a key before being able to set its value.

[christianalfoni]

@Yomguithereal Aha, I see. So actually the highly optimized way would actually be:

// Mutation, no cursor creations
tree.set(['imodels', id], { foo: 'bar'});
tree.push(['items'], 'foo');
tree.merge('config', {});

// Create cursor
tree.select('models');

This way you still have: tree.set(key, value), you reduce amount of cursors created and personally I think it reads better because you instantly see that it is a mutation, not just creating a cursor.

Anyways, this is just food for discussion. I am very happy with the API today :-)

[marbemac]

tree.set(['imodels', id], { foo: 'bar'});

Is this setting imodels>id to the object {foo: 'bar'}? Or is this setting the foo property on imodels>id to 'bar'? If the former, that's the current functionality. If the latter, it's quite confusing - you're putting part of the key in the value slot of .set(key, value).

[Yomguithereal]

tree.set(['imodels', id], { foo: 'bar'});

This replace the node's value at path 'imodels'/id by the { foo: 'bar'} object altogether. Else it would mean you build your path in both arguments, which is obviously silly :).

[marbemac]

@Yomguithereal Agreed, I was just confused because it seems as if @christianalfoni was presenting a new suggestion, but this is the current functionality. Either way, I like the current .set([keyPath], value) signature!

[christianalfoni]

@Yomguithereal @marbemac , does what I stated work today? :-)

[Yomguithereal]

Yes, for the set method only. Polymorphisms for push/merge/etc. remain to be done (I will do it soon).

[christianalfoni]

Ah, okay, cool :-)

[christianalfoni]

Btw, I suppose also the "get" method will get this feature? As you do no really want to create cursors when just fetching some data? Or does it have it already?

[Yomguithereal]

The get method already has it since a long time :) .

[christianalfoni]

Aha :-)

When selectors are ready for the other methods the documentation should have a review concerning this :-)

[Yomguithereal]

Yes indeed. @christianalfoni, I am currently considering to drop the edit method as per your recommendation lately. It would be more coherent, as you said, with the other polymorphisms I am currently introducing for the merge, apply etc. methods.

Therefore we would have:

// Set cursor value
cursor.set(value)
// Set under path
cursor.set('key', value)
cursor.set(['key', 'key2'], value)

// and identically for the others
cursor.merge(object)
cursor.merge(['path'], object)

You are right, the edit method is confusing and I feel it would not be that helpful to stick to zippers' principles here.

[christianalfoni]

Looks very nice :+1:

[Yomguithereal]

The path polymorphism is now effective for every cursor's setters in v1.