inspect-js / is-typed-array

Is this value a JS Typed Array? This module works cross-realm/iframe, does not depend on `instanceof` or mutable properties, and despite ES6 Symbol.toStringTag.
MIT License
12 stars 7 forks source link

Optimize the variable declaration, replacing the use of forEach #15

Closed xguest closed 8 years ago

xguest commented 8 years ago

Need not 'Array.every', a 'Array.find', but for 'object' is no such function, the cycle wrote.

xguest commented 8 years ago

I'm sorry, line 23 of your implementation, check 'if (hasToStringTag && gOPD && Object.getPrototypeOf) {' can not create an object that ignores the function tryTypedArrays.

I have a question Why:

I intentionally return the same function in all cases - since the purpose of this PR is the "for-each" optimization, please revert all unrelated changes.

Please explain why not use: 

value instanceof arr // by constructor
arr.prototype.isPrototypeOf (value) // by prototype
return name === Object(value).constructor.name && (/.*array.*/img).test(Object.prototype.toString.call(value).replace(/\[.+?(\w+)\]/, '$1')); // by name
// function 'name' can be replaced RegExp

Thank you for your hard work

ljharb commented 8 years ago

instanceof and isPrototypeOf and .constructor all don't work if the value comes from, say, an iframe, or a web worker.

xguest commented 8 years ago

I have reviewed your code, it gets the name of choice on the type of 'Symbol.toStringTag' or 'Object.prototype.toString'. All checks are necessary to carry out 'Symbol.toStringTag'. I really do not know much JavaScript, I think you need to add more options for obtaining type name, or select one unique. It was nice to talk to, I would like to continue the dialogue.

P.S. Thank you for your work.

The purest version of your code that I managed to get:

'use strict';

var def = function (value) {
    /* eslint no-return-assign: 1 */
    var typedArrays = /.*((Float|Uint|Int)(8Clamped|8|16|32|64))Array.*/,
        val = typeof value === 'string' ? value : Object.prototype.toString.call(Object(value)),
        out;
    // return TypedArrays name
    return (out = val.replace(typedArrays, '$1')) !== val && out;
    // if need boolean
    /* return typedArrays.test(value); */
};

module.exports = Object.getPrototypeOf && Object.getOwnPropertyDescriptor && typeof Symbol === 'function' && typeof Symbol.toStringTag === 'symbol' ? function (value) {
    var proto, descriptor;
    /* eslint no-return-assign: 1 */
    return Object(value) === value && (
        Symbol.toStringTag in value ? (
                descriptor = Object.getOwnPropertyDescriptor(
                    proto = Object.getPrototypeOf(value),
                    Symbol.toStringTag
                ) || Object.getOwnPropertyDescriptor(
                    Object.getPrototypeOf(proto),
                    Symbol.toStringTag
                )
            ) && descriptor.get && def(descriptor.get.call(value)) : def(value)
  );
} : def;

If you do not need the ternary I can rewrite, or else something. Do you have any suggestions?

ljharb commented 8 years ago

Object property lookups after module parse time are brittle - anyone could overwrite Object.getPrototypeOf or Object.getOwnPropertyDescriptor and break the code. All methods needed should be looked up beforehand, and stored in variables accessible via closure to the exported function.

Some other suggestions are - avoid trying to condense everything to a single expression, and avoid trying to reduce variable usage. More variables are better, both for readability and optimizability. In addition, multiline ternaries should always be avoided.

xguest commented 8 years ago

What if:

module.exports = function () {
    // Perform preliminary checks and return the appropriate function.
    return function () {/* exactly what is needed */}
}();
ljharb commented 8 years ago

There's no need to do that in the IIFE, but sure, that would work - except then you have a different function depending on the environment, which is harder to test, and negligibly faster.

xguest commented 8 years ago

I tried consider all the remarks. If necessary, I can replace the RegExp, on select in the object. There are no mistakes? I can write this PR?

'use strict';

var getprototypeof = require('getprototypeof');

var toStr = Object.prototype.toString;
var hasToStringTag = typeof Symbol === 'function' && typeof Symbol.toStringTag === 'symbol';

var typedArrays = /.*((Float|Uint|Int)(8Clamped|8|16|32|64))Array.*/;
var gOPD = Object.getOwnPropertyDescriptor;

module.exports = function isTypedArray(value) {
    /* eslint max-statements: [0, 15] */
    var out;
    if (Object(value) === value) {
        var proto, descriptor;
        if (hasToStringTag && gOPD) {
            if (Symbol.toStringTag in value) {
                proto = getprototypeof(value);
                descriptor = gOPD(proto, Symbol.toStringTag);
                if (!descriptor) {
                    proto = getprototypeof(proto);
                    descriptor = gOPD(proto, Symbol.toStringTag);
                }
                if (descriptor && descriptor.get) {
                    out = descriptor.get.call(value);
                }
            }
        } else {
            out = toStr.call(value);
        }
    }
    /* return boolean */
    return out && typedArrays.test(out);
    /* if need TypedArrays name or boolean 'false' */
    //  var ret = out.replace(typedArrays, '$1');
    //  return out && ret !== out && ret;
};
xguest commented 8 years ago

Hi! My hold Object.getPrototypeOf?

ljharb commented 8 years ago

This PR title is "replace the use of the forEach" - please restrict the PR to just that change.

It's very hard to review, and understand, a large PR that changes more than one thing. I appreciate the contributions, but I can't safely accept them if they are too risky, and right now this one is too risky.

xguest commented 8 years ago

Hi! I removed check descriptor.get in function

As far as I understand you. If I change forEach on for this good change that has no meaning. If we rewrite the function without cycles is a bad thing:

Tell where and what I can do to you profit and I could then what learn?

xguest commented 8 years ago

You said that before the module.exports a safe space and I can use open functions or constructors. For example: toStrTags [typedArray] = arr.constructor;

It is safe? P.S. This does not apply to the PR

ljharb commented 8 years ago

The "constructor" property isn't safe in any use case.

xguest commented 8 years ago

Hi. Unfortunately, not enough time, work has appeared. Thank you for your hard work. It was nice to chat. Bye.