Extended standard types and Symbol.toStringTag

[dvlsg]

Might be a question more than a bug, but how would you expect this to react?

const type = require('type-detect');
class ExtendedArray extends Array {
  get [Symbol.toStringTag]() {
    return 'ExtendedArray';
  }
}
console.log(type(new ExtendedArray())); //=> 'array'

My initial expectation was that I would receive back ExtendedArray through the use of Symbol.toStringTag, but the code returns array, from here. Is this intended?

[meeber]

imo Symbo.toStringTag should take priority and return "ExtendedArray", which would make this a bug.

@keithamus @lucasfcosta ?

[lucasfcosta]

I totally agree, but apparently, according to the ES7 spec, this is what happens when we call Array.isArray in this line:

If argument is an Array exotic object, return true.

I'm not sure what an "Array exotic object" means exactly, but maybe we could just Object.prototype.toString.call(obj) and them check the index of ' Array]'. This may hurt performance, but it's a safe way to do it. Using isArray or instanceof may return false positives, as it happens on this case.

Maybe we could use isArray and then, if it's true, call Object.prototype.toString.call(obj) just to make sure the current case is not a false positive, this way we would be able to ignore running the latter if our obj is not an array, which would be the majority of cases.

What do you think about this?

PS.: Am I overthinking the problem? 😆

[dvlsg]

Hm. Here's the official spec for "Array exotic objects".

I poked around at a couple options, one of which was this, kept me at about 24,000,000 ops/sec (using Node v6.0.0 for this, by the way):

if (isArrayExists && Array.isArray(obj)) {
  if (symbolToStringTagExists === false || typeof obj[Symbol.toStringTag] === 'undefined') {
    return 'array';
  }
  // return the toString.call(obj) + trims/lowercase from here as a shortcut?
}

This works too, but drops down to about 2,800,000 ops/sec for array literals on my machine:

if (isArrayExists && Array.isArray(obj)) {
  var arrayType = Object.prototype.toString.call(obj);
  if (arrayType === '[object Array]')
    return 'array';
}

I also tried dropping the Array.isArray() call entirely, and instead let it flow to the objPrototype section to see if objPrototype === Array.prototype. That got me about 18,000,000 ops/sec.

The first option above did allow me to drop through to the bottom toString.call() and receive back extendedarray. Here's what I got with a few quick checks:

type(new ExtendedArray()); //=> 'extendedarray'
type([]); //=> 'array'
type(new Array()); //=> 'array'
type(new Uint8Array()); //=> 'uint8array'

Trying to think if that would be sufficient, or if I'm missing a scenario...

Another side note -- is it intended that the custom toStringTag returns are lowercase?

[lucasfcosta]

@dvlsg Whoa! That's great, thanks for your work, it is awesome to have all those pieces of information 😄
And thanks for the spec too, I thought "exotic Array objects" was somewhat of a subjective concept hahahaha

Since numbers don't lie I'd definitely go with the first option. However, I wouldn't return the toString.call inside that conditional branch since we already do that at the end of the whole routine, the way you wrote it seems good to me, it is clear and elegant enough and it is also the most performant option.

I cannot be 100% sure, but I'm 99% sure we do want returns to be lowercase since we have this explicit call right here, unless someone forgot to remove it somewhere along the line I think that is the intended behavior. Since @keithamus has been maintaining this for more time he may be able to either confirm or deny my suspicion.

What do you think @meeber @keithamus ?
If everyone else agrees I'd be happy to review and merge your future PR with this change 😄

[dvlsg]

Cool, I'll hold off on making a PR until we get more feedback from the rest of the team.

For the lowercase toStringTag, I think that's okay if it's intentional, but I noticed the readme.md ends with the following example:

var myObject = {};
myObject[Symbol.toStringTag] = 'myCustomType';
assert(type(myObject) === 'myCustomType');

That should probably have 'mycustomtype' at the end (if the lowercase toStringTag is intentional). I think.

[meeber]

My gut reaction is that we shouldn't lowercase someone's custom toStringTag value. Let's open a separate issue for that.

[lucasfcosta]

@meeber @dvlsg Hmmm, that makes sense, I haven't thought about that. I only thought about Capitalization.
I agree with you guys, we shouldn't use toLowerCase() for this cases.

[keithamus]

Hey @dvlsg this was intended to be the original design (I mean, Array with toStringTag should return the result of that) so I personally would consider this a bug.

The lowercasing is slightly problematic... but it's baked into the design and I feel like we should remain consistent, even if its wrong (because being inconsistent and wrong is worse). We could look at removing the lowercase thing as a breaking change, later down the line - but with 4.x we have many breaking changes and I think I'd sooner ease our users into breaking changes rather than have a whole lump of them.

[dvlsg]

Assuming you aren't already planning on this, how do you feel about also exporting a pre-set chunk of "constants" (for lack of a better word) in a later version (4.x)?

For example, something similar to this:

// assuming `typeDetect()` is the exported function
var types = {
  string: typeDetect(''),
  boolean: typeDetect(true),
  number: typeDetect(1),
  array: typeDetect([]),
  object: typeDetect({}),
  // etc, etc
};
if (setExists)
  types.set = typeDetect(new Set());
module.exports.types = types;

This would allow end users to do something like this:

const { typeDetect, types } = require('type-detect');
typeDetect('str') === types.string; //=> true
typeDetect(new Array()) === types.array; //=> true
typeDetect(null) === types.null; //=> true

// assume assert is chai.assert
assert.typeOf(val, types.error); // or something

That would make case sensitivity... less important? I suppose it would still be at least somewhat relevant (especially in the case of toStringTag).

I could be barking up the entirely wrong tree, though -- figured I'd at least bring up the thought.

[keithamus]

typeDetect itself isn't easily exposed through chai, and I'm not sure it would be easier than doing a find/replace for a breaking change.

I think we could instead look at releasing codemods (scripts which modify users code, like what js-codemod or jscodeshift does) with our breaking changes - so users just have to run a command to have their tests updated to fix breakages. (If you're interested in this concept, read more here: https://medium.com/@cpojer/effective-javascript-codemods-5a6686bb46fb#.dez2cqdbh)

[dvlsg]

Fair enough. And it would have definitely required changes in userland to utilize the constants in existing tests, as well. My thought was that it would protect against further breaking changes in the future, since matching [] to whatever is stored in types.array would (theoretically) alleviate concerns of changing what's stored there (as long the types constants were properly unique, anyways).

But then again, exporting and importing types and looking up types.array is more complicated than just typing 'array', both on the side of an end-user and a maintainer. And you'd still have to use strings for toStringTag anyways, so I can see the other side of the suggestion.

Whatever the case, I think I'm dragging this issue off topic. I'll look into making a PR for the original question (checking for toStringTag inside the isArray() block).