-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat(omit-deep): add function and tests #154
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution!
The docs are generated automatically for every release, so no need to push those in the PR.
function omitDeep(props, input) { | ||
function omitDeepOnOwnProps(obj) { | ||
if (typeof input === 'undefined') { | ||
return input; | ||
} | ||
|
||
if (!Array.isArray(obj) && !isObject(obj)) { | ||
return obj; | ||
} | ||
|
||
if (Array.isArray(obj)) { | ||
return omitDeep(props, obj); | ||
} | ||
|
||
const o = {}; | ||
for (const [key, value] of Object.entries(obj)) { | ||
o[key] = !isNil(value) ? omitDeep(props, value) : value; | ||
} | ||
|
||
return omit(props, o); | ||
} | ||
|
||
if (arguments.length > 2) { | ||
props = Array.prototype.slice.call(arguments).slice(1); | ||
} | ||
|
||
if (Array.isArray(input)) { | ||
return input.map(omitDeepOnOwnProps); | ||
} | ||
|
||
return omitDeepOnOwnProps(input); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you check this? https://github.com/MichaelQQ/deepOmit/blob/master/index.js#L1
} | ||
|
||
if (Array.isArray(input)) { | ||
return input.map(omitDeepOnOwnProps); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be the equivalent of map(omitDeep(['a', 'b', 'c'])
— I don't see much value in supporting an array of objects as input, if that makes sense.
The case of a value within an object being an array should still be handled appropriately, though.
import omitDeep from './omit-deep'; | ||
|
||
describe('omitDeep', () => { | ||
test('omits keys from an object', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also test expected outcomes from non-object-like values: null
, undefined
, false
, true
, new Map()
, new Set()
, number
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having another test where the keys are repeated inside different nested objects would be ideal, as well.
const obj = {
a: 1,
b: 2,
c: {
d: 3,
e: 4
},
g: {
d: 3,
e: 4
},
};
expect(omitDeep(['d', 'e'], obj)).toEqual({ a: 1, b: 2, c: {}, g: {} });
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same goes for keys nested within objects in lists.
const obj = {
a: 1,
b: 2,
c: {
d: 3,
e: 4,
},
j: [
{
d: 44,
e: 23,
f: {
d: 99,
w: 8,
},
},
],
}
expect(omitDeep(['d', 'e'], obj)).toEqual({a: 1, b: 2, c: {}, j: [{ f: { w: 8 } } ]})
return omit(props, o); | ||
} | ||
|
||
if (arguments.length > 2) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the use case of this? I believe we can remove it. There's an expectation that all arguments are always going to be supplied to pure ramda
functions.
expect(omitDeep(['b', 'e'], obj)).toEqual({ a: 1, c: { d: 3 } }); | ||
}); | ||
|
||
test('omits keys from an array of objects', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove this.
return Object.prototype.toString.call(obj) === '[object Object]'; | ||
} | ||
|
||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest we change the jsdoc to something like:
/**
* Recursively omit the specified keys from an object.
* The keys are omitted from the object itself and all its nested objects.
*
* @template T
* @see https://github.com/jonschlinkert/omit-deep/tree/master
* @example
* omitDeep(['b', 'd'], { a: 1, b: 2, c: { d: 3, e: 4 } }); // { a: 1, c: { e: 4 } }
* @param {string[]} props The list of keys to omit.
* @param {T} input The value to omit keys from.
* @returns {T} A new value with the specified keys omitted.
*/
- The function seems to accept any kind of values and it always return the same value passed to it. So a
@template
would be useful here. - The
@param
order looks to be inverted.input
is the second argument to the function. - Would be nice if the
@example
showed the removal of a deeply nested property.
*/ | ||
function omitDeep(props, input) { | ||
function omitDeepOnOwnProps(obj) { | ||
if (typeof input === 'undefined') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These initial conditions could be merged or simplified — i.e.: the Array.isArray
check is carried out several times, in different places.
return input.map(omitDeepOnOwnProps); | ||
} | ||
|
||
return omitDeepOnOwnProps(input); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on all comments above, I think a reasonable rewrite of omitDeep
could look like this.
function omitDeep(props, input) {
if (!isObject(input)) {
return input
}
function omitDeepOwn(obj) {
if (Array.isArray(obj)) {
return obj.map(omitDeepOwn)
}
if (isObject(obj)) {
const o = {}
for (const [key, value] of Object.entries(obj)) {
o[key] = omitDeepOwn(value)
}
return omit(props, o)
}
return obj
}
return omitDeepOwn(input)
}
What do you think?
an alternative for https://www.npmjs.com/package/omit-deep-lodash using ramda.