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

add $delete command to addons.update #2362

Closed
wants to merge 5 commits into from

Conversation

krawaller
Copy link

This PR adds a $delete command to addons.update. It takes a key or an array of keys and deletes them from the target object.

CLA completed (first-time contributor).

@krawaller
Copy link
Author

Ack, only now saw discussion in #1807, where @syranide argues that it is preferable to set the property to undefined.

I'm not sure I agree though - it feels iffy that the property is still lingering. Also with set we can only "remove" one property at a time.

@syranide
Copy link
Contributor

@krawaller "it's generally preferable", if you're dealing with fixed keys then setting to undefined/null should be preferable (to avoid creating new hidden classes), but if you're dealing with dynamic indices then deleting probably makes most sense. It was just intended as a reminder for anyone stumbling upon it.

@krawaller
Copy link
Author

Nod, understood.

Another note, just realised that to continue parity with MongoDB, this should probably be renamed to unset and altered to behave in the same way?

@krawaller
Copy link
Author

Updated functionality for MongoDB $unset parity (but haven't changed name):

  • Target object can now be an array, in which case we replace matching element(s) with null.
  • Also argument can be an object with keys to be deleted.
  • EDIT: change name from delete to unset.

This means we now have the exact same functionality as MongoDB, except...

  • we can give a single key or an array or object of keys, while in mongo its always an object
  • we don't support dot notation in keys which Mongo does

@chenglou
Copy link
Contributor

Nagging @petehunt because we wanna pull this in and deprecate update in favour of immutable-js

@gaearon
Copy link
Collaborator

gaearon commented Jan 22, 2015

I've said it here but want to bring the comment here as well: I'm not sure special behavior for arrays is justified, even if Mongo does it.

IMO if update lets your set not-yet-specified keys on an array with $set, then it should be possible to use $unset to undo this. A common use case for update is changing data (e.g. in Flux store), and when you change data, you often want to be able to revert your changes later.

@gaearon
Copy link
Collaborator

gaearon commented Jan 22, 2015

On the other hand, most people probably want “normal” $unset behavior that doesn't make array sparse (this also has bad perf effects IIRC). So I get why Mongo did that.

After all, if you really wanted to poke a hole in an array, you can $apply and do that manually.

@flying-sheep
Copy link

this also has bad perf effects IIRC

it’s the other way round: dense arrays have bad performance characteristics when used to represent sparse ones. and nothing about setting elements to null in an array is in any way more normal than setting it to 'thingamajig'

mongo’s behavior makes no sense:

When used with $ to match an array element, $unset replaces the matching element with null rather than removing the matching element from the array. This behavior keeps consistent the array size and element positions.

that’s bullshit. if you delete someArray[index], you’ll still be able to get the index after the last element with someArray.length afterwards, and of course the element positions stay the same. so why the hell would it set array elements to null?

also, why would someone specify an object without values just to give a set of keys. JS has no native syntax for sets (objects-without-values), and we only iterate the keys, so KISS and simply accept an array of keys or one key here like in other update APIs:

update([1,2,3], {$unset: 1}) === [1,,3]
update([1,2,3], {$unset: [0, 2]}) === [,2]
Object.keys(update([1,2,3], {$unset: 0})) === ['1', '2']

@b5
Copy link

b5 commented Oct 6, 2015

+1 for this. I'd love an $unset command!

@CosticaPuntaru
Copy link

+1

@sophiebits
Copy link
Collaborator

Prefer using immutable.js. We don't want to turn this into a crazy DSL.

@sophiebits sophiebits closed this Oct 14, 2015
@dustingetz
Copy link

Is this even used internally, can we split this out into a standalone library? addons.update is valuable to me outside of React projects.

@chenglou
Copy link
Contributor

@sophiebits
Copy link
Collaborator

@dustingetz Would you want to maintain it?

@zpao
Copy link
Member

zpao commented Oct 14, 2015

It is used internally (unfortunately). And yes, while it's available standalone(ish, it still depends on react), it should be split off in a real standalone project. Give me a good name and I'll do it sometime soon. I kinda want to call it ognom

@dustingetz
Copy link

I want to use it to implement update-in for vanilla js data structures, which would let us compose actions. It's so easy to implement that you could even just include the sugar and call it updateIn 😄

@peteruithoven
Copy link

I see it's split on npm: https://www.npmjs.com/package/react-addons-update, but where does the code live? Is there a seperate github repo? I can only find the code in the react repo?
https://github.com/facebook/react/blob/master/src/addons/update.js

@zpao
Copy link
Member

zpao commented Oct 28, 2015

@peteruithoven It currently only lives here, where you found it. The npm module (all of the react-addons- modules in fact) just require deeply into react for the time being until they can become standalone / get removed.

@peteruithoven
Copy link

But pull requests like this one aren't merged? It feels like we shouldn't use this until it's split off and it can accept more contributors / contributions.

@gaearon
Copy link
Collaborator

gaearon commented Oct 29, 2015

From what I understand it stays here in 0.14 because react/addons still works for back compat.
0.15 is the release that kills 0.14 back compat, so I'd expect it to become standalone at that point.

@dustingetz
Copy link

As an experiment here is updateIn implemented against react-addons-update, along with other nice wrappers, it is a very nice programming model https://github.com/dustingetz/update-in

import {updateIn, merge, push, unshift, splice} from 'update-in';
const val = {a: {b: 0, c: 2}, xs: [1, 2]};

merge({x: 1, y: 1}, {y: 2, z: 2}) // => {x: 1, y: 2, z: 2}
updateIn(val, ['a'], merge, {c:99, d: 99})     // => {a: {b: 0, c: 99, d: 99}, xs: [1, 2]}

@ahdinosaur
Copy link

👍, being able to delete a property is necessary when you want a property to no longer exist when iterating with for ... in, Object.keys.

by the way, if anyone wants a react-addons-update compatible module that provides this ability, tcomb/lib/update has $remove which does the same.

@gaearon
Copy link
Collaborator

gaearon commented Apr 1, 2016

You might be interested in https://github.com/kolodny/immutability-helper that lets you define custom operations by name.

@facebook-github-bot
Copy link

@krawaller updated the pull request.

@hoytech
Copy link

hoytech commented Apr 19, 2017

Using $apply and/or immutable.js doesn't work for some use-cases since updates can't be serialised for transfer between client and server (short of implementing another protocol on top).

In case it helps anyone, I have created an almost-compatible module which implements $unset:

https://www.npmjs.com/package/update-immutable

My module also supports autovivification which can help you avoid maintaining initial state skeletons in components.

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.