add u.omitted constant #78
Conversation
README.md
Outdated
| ```js | ||
| var user = { email: 'john@aol.com', username: 'john123', authToken: '1211..' }; | ||
|
|
||
| var result = u({ authToken: _.omitted, active: true }, user); |
There was a problem hiding this comment.
urgh. u.cantbelievehowoftenImakethatypo
| @@ -414,6 +426,7 @@ function isSensitive(value, key) { return key == 'SSN' } | |||
| var result = u({ user: u.omitBy(isSensitive) }, user); | |||
|
|
|||
| expect(result).to.eql({ user: { email: 'john@aol.com', username: 'john123', authToken: '1211..' } }); | |||
|
|
|||
There was a problem hiding this comment.
Yeah, eslint got overzealous. All spurious style changes should have been rolled back now.
lib/update.js
Outdated
| import wrap from './wrap' | ||
| import constant from './constant' | ||
|
|
||
| const innerOmitted = constant('omitted') |
There was a problem hiding this comment.
I think this can just be {} since that will work for reference equality. Making it a function that returns the string omitted doesn't seem necessary unless I'm missing something.
There was a problem hiding this comment.
No, you're right, any reference would do. I was just trying to put a reference that could be potentially more telling than an empty object, but that will work just as well.
There was a problem hiding this comment.
Perhaps something like { __omitted: true }. Still a reference, just more meaningful if seen in debug
There was a problem hiding this comment.
Also, there are some dangers to relying on reference. If a person has two versions of updeep in their tree, it's possible that this won't work properly. Maybe it should be modeled after however lodash does their placeholder? I don't remember how off the top of my head.
There was a problem hiding this comment.
[lodash] Hmmm... I'll have a look.
There was a problem hiding this comment.
Although I think the ref won't work only if somebody does:
import u1 from 'updeep';
import u2 from './path/to/other/updeep';
u1({ foo: u2.omitted })(x);
which would be rather twisted.
There was a problem hiding this comment.
agree that it's unlikely, if someone has a library that depends on updeep to build custom operators you could get into this situation. That library may export an operator that uses omitted from its version of updeep and if you use it with yours...
Not likely, I agree, but if lodash has an easy way to deal w/ it, it's probably good to follow that pattern.
There was a problem hiding this comment.
And unless I'm mistaken, lodash does it by attaching a placeholder attribute to the main exported _ variable: https://github.com/lodash/lodash/blob/4ea8c2ec249be046a0f4ae32539d652194caf74f/.internal/getHolder.js
We should be able to do the same. Give me 5 minutes. :-)
There was a problem hiding this comment.
Darn. update is wrapped via wrap before it's exported, so binding would be a little more involved than I thought. And I'm not sure it'd bring any more safety than using the omitted and update from the same u source.
There was a problem hiding this comment.
okay, let's just try a reference for now.
lib/update.js
Outdated
| @@ -19,7 +25,9 @@ function resolveUpdates(updates, object) { | |||
| let updatedValue = value | |||
|
|
|||
| if ( | |||
| !Array.isArray(value) && value !== null && typeof value === 'object' | |||
| !Array.isArray(value) && | |||
There was a problem hiding this comment.
I'm down with prettiering the repo, but I'd prefer it be a separate commit/PR. Could you please amend any of the non-essential changes out?
| }, { a: 1, b: 2 }) | ||
|
|
||
| expect(result).to.eql({ b: 3 }); | ||
| }) |
There was a problem hiding this comment.
Please include a test for a nested object and for an array (what should it do if you omit in an array?)
There was a problem hiding this comment.
tests added.
The _.omitted should skip the property value, no matter if it's a scalar or an array or object, so we should be good there.
The undefined case is if one does:
u({ 3: u.omitted })([ 1,2,3,4,5 ]);
I could add logic to take care of that case, basically do
if (Array.isArray(defaultedObject)) {
return updateArray(resolvedUpdates, defaultedObject).filter( item => item !== innerOmitted )
}
but it just doesn't feel like something anybody would do.
There was a problem hiding this comment.
Yeah, I guess the question is, what should the lib do... throw? Create a sparse array? filter it out as you demonstrate there?
Maybe throwing for now in updateArray would work.
There was a problem hiding this comment.
I went with filtering, as one can already do the sparse-y array via u({ 3 => null }), and between throwing an exception and doing something maybe possibly useful in some cases, the latter sounds a smidge better. :-)
|
looks like there are some formatting errors, could you run prettier/fix those up, please? |
|
done! |
|
Could you rebase on master when you get a chance please? I think that'll fix the build. |
|
done! |
|
Looks great, thank you for your contribution! |
|
Released as 1.1.0, thanks again. |
|
Pleasure was all mine! Thanks! :-) |
Right now if one wants to remove properties and update others, we need to do it in two steps: one call to
u.omitand one tou.update. This PR adds au.omittedconstant signalling that the property should be omitted (we can't usenullorundefinedas those could be legit property values we might want to keep).