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.

Sign up for GitHub

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

Jump to bottom

Feature/set #2961

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Feature/set #2961

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
23e2515
added new method `set`
AzatCoder Jul 18, 2022
2e38b16
added tests for `set`
AzatCoder Jul 19, 2022
891f79a
little comment for `set`
AzatCoder Jul 19, 2022
7c6c6a1
review
AzatCoder Jul 22, 2022
b5f481a
documentation
AzatCoder Jul 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 20 additions & 0 deletions modules/_set.js
View file
Open in desktop
@@ -0,0 +1,20 @@
import isNumber from './isNumber.js';
import isArray from './isArray.js';
import isObject from './isObject.js';


export default function set (obj, path, value) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Three changes needed for this internal function as a whole:

  • It is not used anywhere except in the public set, so it does not need to be in a separate module (compare isEqual). Please move it into ./set.js. You can call it deepSet (cf. deepGet).
  • Please add a top comment with a description, like all other functions.
  • (minor) The general convention in Underscore is to have no space between the function name and the parameter list, so please remove it.
Suggested change
export default function set (obj, path, value) {
export default function set(obj, path, value) {

var key = String(path[0]);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think calling String adds any value here.


if (path.length === 1) {
obj[key] = value;
return;
}

if (!isArray(obj[key]) || !isObject(obj[key])) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This condition doesn't quite do what you want. Lifting the negation, you get !(isArray(obj[key]) && isObject(obj[key])). However, isArray(obj) implies isObject(obj), so this entire expression reduces to just !isArray(obj[key]). In other words, this block is executed if obj[key] is any non-array value, even if it is an object.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, my bad. It should be if (!isObject(obj[key])) {...}

var nextKey = path[1];
obj[key] = isNumber(nextKey) ? [] : {};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isNumber('123') returns false, which is not what you want in this case. You need to match nextKey against a regular expression, as I demonstrated in #2948.

}

return set(obj[key], path.slice(1), value);
}
1 change: 1 addition & 0 deletions modules/index.js
View file
Open in desktop
Expand Up @@ -62,6 +62,7 @@ export { default as tap } from './tap.js';
export { default as get } from './get.js';
export { default as has } from './has.js';
export { default as mapObject } from './mapObject.js';
export { default as set } from './set.js';

// Utility Functions
// -----------------
Expand Down
14 changes: 14 additions & 0 deletions modules/set.js
View file
Open in desktop
@@ -0,0 +1,14 @@
import isArray from './isArray.js';
import isObject from './isObject.js';
import _set from './_set.js';


// set the value in given path
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Underscore is literate code. Please elaborate a bit and write full sentences.

export default function set (obj, path, value) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export default function set (obj, path, value) {
export default function set(obj, path, value) {

if (!isObject(obj) || !isArray(path)) return obj;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By convention, Underscore functions allow you to pass a single path component as a bare string or number instead of an array. Take path through _.toPath instead of rejecting it if it is not an array.

if (path.length === 0) return obj;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're checking path.length here and then again in the internal set function. Not a big deal, but slightly inefficient.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I don't see other option better than this. I like this implementation. Please let me know if you have an idea how to improve it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please refer to the approach I suggested in #2948. If you do postorder assignment instead of preorder assignment, you can check for path.length === 0 only in the internal function and immediately return value if true. You can avoid lookahead beyond path[0] in that case by replacing obj instead of obj[key].

Referring again to #2948, I also realize that your code is still vulnerable to prototype pollution. You need to fix that as well.


_set(obj, path, value);

return obj;
}
25 changes: 25 additions & 0 deletions test/objects.js
View file
Open in desktop
Expand Up @@ -1255,4 +1255,29 @@
assert.deepEqual(_.mapObject(protoObj, _.identity), {a: 1}, 'ignore inherited values from prototypes');

});


QUnit.test('set', function(assert) {
// returns first argument if first argument is primitive type
assert.strictEqual(_.set(undefined, ['some', 'path'], 'some value'), undefined);
assert.strictEqual(_.set(null, ['some', 'path'], 'some value'), null);
assert.strictEqual(_.set(12, ['some', 'path'], 'some value'), 12);
assert.strictEqual(_.set(BigInt(1), ['some', 'path'], 'some value'), BigInt(1));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are trying to support old environments, so you cannot just sprinkle post-ES3 features in the tests (or the source code, for that matter). To fix, make this line conditional on feature detection:

Suggested change
assert.strictEqual(_.set(BigInt(1), ['some', 'path'], 'some value'), BigInt(1));
if (typeof BigInt != 'undefined') {
assert.strictEqual(_.set(BigInt(1), ['some', 'path'], 'some value'), BigInt(1));
}

You'll find similar code elsewhere in this module.

assert.strictEqual(_.set('string', ['some', 'path'], 'some value'), 'string');
assert.strictEqual(_.set(true, ['some', 'path'], 'some value'), true);

// returns the same object if path is not an array or its empty
assert.deepEqual(_.set({random: {object: true}}, undefined, 'some value'), {random: {object: true}});
assert.deepEqual(_.set({random: {object: true}}, {}, 'some value'), {random: {object: true}});
assert.deepEqual(_.set({random: {object: true}}, [], 'some value'), {random: {object: true}});

assert.deepEqual(_.set({x: {l: 10}}, ['x', 'l'], 'changed'), {x: {l: 'changed'}});
assert.deepEqual(_.set({x: 10}, ['my', 'path'], 'my value'), {x: 10, my: {path: 'my value'}});
assert.deepEqual(_.set({x: 10}, ['my', 0, 'path'], 'my value'), {x: 10, my: [{path: 'my value'}]});
assert.deepEqual(_.set({x: 10}, [0], 'my value'), {x: 10, '0': 'my value'});
assert.deepEqual(_.set({x: 10}, [0, 1], 'my value'), {x: 10, '0': [undefined, 'my value']});
assert.deepEqual(_.set({x: 10}, [0, 0], 'my value'), {x: 10, '0': ['my value']});
assert.deepEqual(_.set({x: 10}, [0, '0'], 'my value'), {x: 10, '0': {'0': 'my value'}});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree with this difference. Numeric keys are implicitly converted to strings. Array indices are actually strings, too. The second test should generate the same value as the first.

Suggested change
assert.deepEqual(_.set({x: 10}, [0, 0], 'my value'), {x: 10, '0': ['my value']});
assert.deepEqual(_.set({x: 10}, [0, '0'], 'my value'), {x: 10, '0': {'0': 'my value'}});
assert.deepEqual(_.set({x: 10}, [0, 0], 'my value'), {x: 10, '0': ['my value']});
assert.deepEqual(_.set({x: 10}, [0, '0'], 'my value'), {x: 10, '0': ['my value']});

assert.deepEqual(_.set({x: 10}, [0, 'my'], 'my value'), {x: 10, '0': {my: 'my value'}});
});
}());
29 changes: 28 additions & 1 deletion underscore-esm.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion underscore-esm.js.map
View file
Open in desktop

Large diffs are not rendered by default.

28 changes: 28 additions & 0 deletions underscore-node-f.cjs
View file
Open in desktop
Expand Up @@ -763,6 +763,32 @@ function mapObject(obj, iteratee, context) {
return results;
}

function set$1 (obj, path, value) {
var key = String(path[0]);

if (path.length === 1) {
obj[key] = value;
return;
}

if (!isArray(obj[key]) || !isObject(obj[key])) {
var nextKey = path[1];
obj[key] = isNumber(nextKey) ? [] : {};
}

return set$1(obj[key], path.slice(1), value);
}

// set the value in given path
function set (obj, path, value) {
if (!isObject(obj) || !isArray(path)) return obj;
if (path.length === 0) return obj;

set$1(obj, path, value);

return obj;
}

// Predicate-generating function. Often useful outside of Underscore.
function noop(){}

Expand Down Expand Up @@ -1919,6 +1945,7 @@ var allExports = {
get: get,
has: has,
mapObject: mapObject,
set: set,
identity: identity,
constant: constant,
noop: noop,
Expand Down Expand Up @@ -2134,6 +2161,7 @@ exports.rest = rest;
exports.restArguments = restArguments;
exports.result = result;
exports.sample = sample;
exports.set = set;
exports.shuffle = shuffle;
exports.size = size;
exports.some = some;
Expand Down
2 changes: 1 addition & 1 deletion underscore-node-f.cjs.map
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion underscore-node.mjs
View file
Open in desktop
Expand Up @@ -3,5 +3,5 @@
// (c) 2009-2022 Jeremy Ashkenas, Julian Gonggrijp, and DocumentCloud and Investigative Reporters & Editors
// Underscore may be freely distributed under the MIT license.

export { VERSION, after, every as all, allKeys, some as any, extendOwn as assign, before, bind, bindAll, chain, chunk, clone, map as collect, compact, compose, constant, contains, countBy, create, debounce, _ as default, defaults, defer, delay, find as detect, difference, rest as drop, each, _escape as escape, every, extend, extendOwn, filter, find, findIndex, findKey, findLastIndex, findWhere, first, flatten, reduce as foldl, reduceRight as foldr, each as forEach, functions, get, groupBy, has, first as head, identity, contains as include, contains as includes, indexBy, indexOf, initial, reduce as inject, intersection, invert, invoke, isArguments, isArray, isArrayBuffer, isBoolean, isDataView, isDate, isElement, isEmpty, isEqual, isError, isFinite, isFunction, isMap, isMatch, isNaN, isNull, isNumber, isObject, isRegExp, isSet, isString, isSymbol, isTypedArray, isUndefined, isWeakMap, isWeakSet, iteratee, keys, last, lastIndexOf, map, mapObject, matcher, matcher as matches, max, memoize, functions as methods, min, mixin, negate, noop, now, object, omit, once, pairs, partial, partition, pick, pluck, property, propertyOf, random, range, reduce, reduceRight, reject, rest, restArguments, result, sample, filter as select, shuffle, size, some, sortBy, sortedIndex, rest as tail, first as take, tap, template, templateSettings, throttle, times, toArray, toPath, unzip as transpose, _unescape as unescape, union, uniq, uniq as unique, uniqueId, unzip, values, where, without, wrap, zip } from './underscore-node-f.cjs';
export { VERSION, after, every as all, allKeys, some as any, extendOwn as assign, before, bind, bindAll, chain, chunk, clone, map as collect, compact, compose, constant, contains, countBy, create, debounce, _ as default, defaults, defer, delay, find as detect, difference, rest as drop, each, _escape as escape, every, extend, extendOwn, filter, find, findIndex, findKey, findLastIndex, findWhere, first, flatten, reduce as foldl, reduceRight as foldr, each as forEach, functions, get, groupBy, has, first as head, identity, contains as include, contains as includes, indexBy, indexOf, initial, reduce as inject, intersection, invert, invoke, isArguments, isArray, isArrayBuffer, isBoolean, isDataView, isDate, isElement, isEmpty, isEqual, isError, isFinite, isFunction, isMap, isMatch, isNaN, isNull, isNumber, isObject, isRegExp, isSet, isString, isSymbol, isTypedArray, isUndefined, isWeakMap, isWeakSet, iteratee, keys, last, lastIndexOf, map, mapObject, matcher, matcher as matches, max, memoize, functions as methods, min, mixin, negate, noop, now, object, omit, once, pairs, partial, partition, pick, pluck, property, propertyOf, random, range, reduce, reduceRight, reject, rest, restArguments, result, sample, filter as select, set, shuffle, size, some, sortBy, sortedIndex, rest as tail, first as take, tap, template, templateSettings, throttle, times, toArray, toPath, unzip as transpose, _unescape as unescape, union, uniq, uniq as unique, uniqueId, unzip, values, where, without, wrap, zip } from './underscore-node-f.cjs';
//# sourceMappingURL=underscore-node.mjs.map
27 changes: 27 additions & 0 deletions underscore-umd.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion underscore-umd.js.map
View file
Open in desktop

Large diffs are not rendered by default.