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

addons.update directive precedence order #1615

Closed
DalekBaldwin opened this issue May 28, 2014 · 8 comments
Closed

addons.update directive precedence order #1615

DalekBaldwin opened this issue May 28, 2014 · 8 comments

Comments

@DalekBaldwin
Copy link

var an_array = [0,1,2,3,4,5];
React.addons.update(an_array, {4: {$set: '$set'}, $push: ['p','u','s','h'], $unshift: ['u','n','s','h','i','f','t'], $splice: [[1,1,'$splice','slice','dice']]});

// result:
["t", "$splice", "slice", "dice", "$set", "h", "s", "n", "u", 0, 1, 2, 3, 4, 5, "p", "u", "s", "h"]

Is this a sane ordering for handling these directives? I'd never want to have update recurse into something I've already $pushed, $unshifted, or $spliced in -- otherwise I would have included the changes in those directives' arguments originally. I think the most natural expectation would be that update should recurse into existing elements at their indices in the original array, then splice in the $splice directive, then $push and $unshift new elements to ensure they appear at the ends of the resulting array, like this:

["t", "f", "i", "h", "s", "n", "u", 0, "$splice", "slice", "dice", 2, 3, "$set", 5, "p", "u", "s", "h"]

As it is now, for certain cases, the programmer would have to introduce difficult-to-visually-inspect index-twiddling logic or split the changes up among multiple calls to update to produce the desired result.

The tests for update don't include any cases where multiple directives appear for a single value, so I'm not sure whether this is intentional or whether anybody depends on this behavior. But especially when you're dealing with trees of this form:

{...
 children:
 [{...
   children:
   [{...
     children:
     [...]
    }
    ...]
  }]}

the current behavior really doesn't allow for the same visual clarity that makes update so helpful for trees composed entirely of objects.

@sophiebits
Copy link
Collaborator

My first instinct would be to say that you can use exactly one directive or do nested updates, not both.

@syranide
Copy link
Contributor

Something I've been fiddeling with myself: https://gist.github.com/syranide/0d0c7798c18be3f8a243 (not finished)

Might be an interesting alternative, avoids the use of directives in favor of regular manipulation.

@pedroteixeira
Copy link

There's lots of interest in coming up with better (user) solutions to this problem.
Right now, I'm experimenting using json paths as an alternative , example:

ref = Ref({})
ref.set_in('a/children/0/children/2', 2)

would set the new value according to existing path. When the path does not exist, it's possible to build correct nodes (object or array) according to a json schema if available.

Instead of trying to combine multiple 'update operations' in a single line, I think it's best to just have a different mecanism for batching/transacting the multiple operations when that's desired, such as
ref.transact(function() { ... }).

@slorber
Copy link
Contributor

slorber commented Jul 24, 2014

@pedroteixeira I agree with this and have build some similar path mecanism instead of using an "object path".

@chenglou
Copy link
Contributor

Since we're deprecating Update in favour of immutable-js, I think, beside existing bug fixes + some low hanging feature requests, we won't be adding a whole new syntax for updating nested collections like json paths.

(FYI, immutable-js and Clojure uses ['path', 'to', 'child'], which is imo a way saner way of approaching this.)

That being said, I agree with @spicyj. Yes, the ordering is arbitrary right now, but you probably shouldn't rely on it the same way you can't do {$push: [1], $push: [2]}, if you know what I mean.

If that makes sense, can we close this? (@petehunt)

@syranide
Copy link
Contributor

(FYI, immutable-js and Clojure uses ['path', 'to', 'child'] , which is imo a way saner way of approaching this.)

@chenglou It is not GCC ADVANCED_MODE-compatible though I think?

@slorber
Copy link
Contributor

slorber commented Oct 31, 2014

@pedroteixeira if you are interested I build my framework around Om/cursor concepts and used React update and paths.

It is probably not the most efficient and clean code but it does the job well and is in production :)
Check that:
https://github.com/stample/atom-react/blob/master/src/atom/atomUtils.js
This is the code used inside cursor.set and cursor.get operations

My Atom implementation support transactions and allow to do something like this on dom events:

        this.transact(function () {
            this.props.cursor1.set("newValue");
            this.props.cursor2.set("newValue2");
        }.bind(this));

During a transaction, you can read your writes.
The drawback of my current implementation is that is generates more intermediary objects during the transaction

@gaearon
Copy link
Collaborator

gaearon commented Apr 1, 2016

We are handing over update() to the community so I’m closing this.
Please see my note in #6353 (comment).

@gaearon gaearon closed this as completed Apr 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants