3
\$\begingroup\$

I have written a simple JavaScript function to make an array reactive (i.e. observable). Is there anything I could improve? What the code does is take an array, a callback and a this value as arguments, then whenever the array gets updated or a value gets added to it, it runs the callback with the specified this value. It works similar to the obsolete Array.observe().

function arrayObserve(array, callback, thisArg){
    var result = [];
    result.push.apply(result, array);
    function getterAndSetter() {
        if (Array.isArray(array))
            var currentArray = result;
        else
            var currentArray = Object.keys(result);
        console.log(result);
        currentArray.forEach( function(prop){
            if (Array.isArray(result))
                var currentProp = result.lastIndexOf(prop);
            else
                var currentProp = prop;

            Object.defineProperty(array, currentProp, {
                configurable: true,
                enumerable: true,
                set: function(val) {
                    result[currentProp] = val;
                    callback.call(thisArg, result);
                },
                get: function() {
                    return result[currentProp];
                }
            });
            //}
            if (typeof result[currentProp] == 'object') {
                arrayObserve(result[currentProp], callback, result);
            }
        });
    }
    getterAndSetter();
    ['pop','push','reverse','shift','unshift','splice','sort','map','filter'].forEach( function(method){
        Object.defineProperty(array, method, {
            configurable: false,
            enumerable: false,
            writable: true,
            value: function () {
                var noReturnMethods = ['push', 'pop', 'reverse', 'shift', 'unshift'];
                if (noReturnMethods.indexOf(method) > -1) {
                    Array.prototype[method].apply(result, arguments);
                } else {
                    var results = Array.prototype[method].apply(result, arguments);
                    result = results;
                }
                callback.call(thisArg, result);
                getterAndSetter();
                return result;
            }
        });
    });
    return result;
}
\$\endgroup\$
2
+50
\$\begingroup\$

With this block:

if (typeof result[currentProp] == 'object') {
    arrayObserve(result[currentProp], callback, result);
}

It appears that the code is handling nested arrays. Is that supposed to work with nested objects (A.K.A. associative arrays)? If not, then perhaps it would be better to use Array.isArray() like the start of getterAndSetter().


The if-else logic within getterAndSetter() doesn't put the statements in a block (i.e. in curly brackets - { and }), whereas the if-else logic within the value function does put the statements in blocks. For the sake of readability (especially for others) it is best to be consistent with curly brackets style.


In the else block of the value function, when a return method is called:

var results = Array.prototype[method].apply(result, arguments);
result = results;

is results used to avoid side-effects of the method (e.g. splice)? If not, then there isn't really a need to use results - just re-assign result with the return value from calling apply on the method.


Perhaps you are already aware of this, but if I add a new element to an array using bracket notation after arrayObserve has already been called with the array as the first parameter, then the callback is not called - should that be the case?

var arr = [3, 5, [1]];
arrayObserve(arr, console.log.bind(null,'callback - arguments:'));
arr[3] = 4;

Edit

One other thing to consider is using Array.from() when creating result, unless a shallow-copy won't work. That could simplify the following first two lines of arrayObserve():

var result = [];
result.push.apply(result, array);

To this:

var result = Array.from(array);

If a deep-copy is desired, then the trusty JSON methods can be utilized:

var result = JSON.parse(JSON.stringify(array));
\$\endgroup\$
  • 1
    \$\begingroup\$ I have accepted this answer because it helped me the most, thank you for the answer. \$\endgroup\$ – Jacques Marais Jul 10 '18 at 16:55
2
\$\begingroup\$

Some minor nit-picks about your code.

Object.defineProperty(array, method, {
    configurable: false,
    enumerable: false,
    writable: true,
    value: function () {
        // ...

Here, configurable and enumerable default to false anyway. You needn't set them explicitly.

if (noReturnMethods.indexOf(method) > -1) {
    Array.prototype[method].apply(result, arguments);
}

With ES6, you can write the if condition more concisely as noReturnMethods.includes(method).

You could improve readability by storing ['pop','push','reverse','shift','unshift','splice','sort','map','filter'] in a variable. You could probably do

var noReturnMethods = ['push', 'pop', 'reverse', 'shift', 'unshift'];
var returnMethods = ['splice','sort','map','filter'];
var methods = [];
methods = methods.push.apply(noReturnMethods, returnMethods);

... to avoid duplication of the names of methods.

With ES6, you could do methods = [...noReturnMethods, ...returnMethods].

Assuming your environment supports ES6, you should probably make use of arrow functions and the let and const keywords.

\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks for the answer. My environment doesn't necessarily support ES6, but will keep those in mind. If it did, I would probably use Proxies to achieve this. \$\endgroup\$ – Jacques Marais Jul 8 '18 at 19:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.