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

Correctly handle remove from an array #23

Merged
merged 3 commits into from
Oct 17, 2016
Merged

Correctly handle remove from an array #23

merged 3 commits into from
Oct 17, 2016

Conversation

IvanGoncharov
Copy link
Contributor

Currently remove use delete parent[finalToken] both object and arrays.
Here is how JS interperet delete on array:

$ node
> a = [1,2,3]
[ 1, 2, 3 ]
> delete a[1]
true
> a
[ 1, , 3 ]
> JSON.stringify(a)
'[1,null,3]'

So instead of deleting element delete replace it with null.

@manuelstofer Can you please review this PR?

@IvanGoncharov
Copy link
Contributor Author

@manuelstofer Can you please review this PR?

@IvanGoncharov
Copy link
Contributor Author

@manuelstofer This PR blocks my project. Can you pleaser review it?

@manuelstofer manuelstofer merged commit cbf06fb into manuelstofer:master Oct 17, 2016
@epoberezkin
Copy link
Collaborator

This change may break backward compatibility for lots of people. It's definitely not a bug that array element is removed without changing indices of other elements. Arguably, it's a feature.
So I think it should be deployed as a new major version (can be 0.7.0) and also needs documenting.

@epoberezkin
Copy link
Collaborator

epoberezkin commented Oct 17, 2016

It think it could be better to make it an option, some people may still need old functionality for arrays.

@epoberezkin
Copy link
Collaborator

Sorry, I see it is a new version. Still a comment that old functionality may be needed still applies.

@manuelstofer
Copy link
Owner

Yes, maybe I merged it a bit too fast. Might have been better to add a second function for that behavior.

@epoberezkin I currently don't have too much time to maintain the library. Would you be interested?

@epoberezkin
Copy link
Collaborator

I can keep an eye on it, wouldn't want to be the sole maintainer though, it's still your baby :)
Please add me to admins here and on npm so I can push/merge/publish and I will do it.
I agree, a separate method "removeItem" is better than the option.
@IvanGoncharov would you be able to do it so in 0.7.0 remove does what it did before and a new method is added to do what you need?

@IvanGoncharov
Copy link
Contributor Author

@epoberezkin It's easy to add new and will create new PR.
But I think the main issue here is how unintuitive current behavior is especially when after stringify back to JSON. I don't think other developers expect properties to be removed from an object but array items to be replaced with null.

+ I think removeItem is misleading since it will work with object properties not only items.
IMHO, default remove should have consistent behavior for both arrays and objects.
It's better to create separate delete function which will use JS delete operator over elements both in arrays and objects.

@IvanGoncharov
Copy link
Contributor Author

+ I can add delete as 0.6.1 without introducing more breaking changes.

@epoberezkin
Copy link
Collaborator

ok. And also please add a notice in readme about the change in "remove" functionality for arrays in 0.6.0

@manuelstofer
Copy link
Owner

Sounds good.

@epoberezkin I added you on to the git repository and on npm

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

Successfully merging this pull request may close these issues.

None yet

3 participants