fix: favor extended array toStringTag where available

[dvlsg]

Discussion in #49

Still pending any specific tests to cover this scenario. If you'd like me to add tests as part of this pull request, let me know where you'd like them. (new-ecmascript-types.js?)

[keithamus]

Hey @dvlsg, thanks for this. Looks great!

Tests would definitely be ideal before we merge. We have toStringTag tests here in test/index.js#L227-261. If you could add Arrays there, that'd be great. Thanks!

[dvlsg]

Can do.

[dvlsg]

Is it safe for me to assume that test/index.js can be run in environments that may not support classes?

Considering adding checks like found here. Are those worthwhile in this case, or just extra noise?

[keithamus]

@dvlsg yes indeed. The tests will be run in IE9, PhantomJS, etc. classes are not a given. Feel free to wrap it in the style we have done, such as in the example you linked.

[dvlsg]

Hm. Thoughts on the Node v4.3 case? It's coming out as [object Array] with the code I have. I can take a closer look later on, as well.

[keithamus]

These ifs could be combined for terseness.

[dvlsg]

All makes sense to me, here's another swing at it, might not be 100% there yet.

Couple notes about what I did:

• I think you're right about Symbol.toStringTag being a function. I know it works as a getter (for whatever reason), but a function is probably the more right way, so I made that swap.
• I swapped the logic in index.js to check if Symbol exists instead of if Symbol.toStringTag exists. I'm hoping that's okay for all 4.0 branches, like the announcement suggests. Seems to be okay for 4.3, at any rate. Otherwise, the logic I originally had was failing the symbolToStringTagExists check and sending back array instead of dropping through to give back extendedarray. The change allows the unit tests to pass, but it's a bit awkward at best, especially considering that I believe this would be checking typeof array[undefined] on node 4.3. I'm all ears if you have a better suggestion for covering that scenario.
• I combined the if statements per your suggestion, and took a stab at what sort of formatting you'd want for a multiline if (assuming you'd even want a multiline if...) since putting it all on one line made the linter sad. (I think it was at 123 characters, so close!)

[meeber]

I believe the symbolToStringTagExists check is necessary anywhere .toStringTag is used. There are some browser versions that support Symbol but not Symbol.toStringTag which means this:

(symbolExists === false || typeof obj[Symbol.toStringTag] === 'undefined')

...ends up meaning this:

(symbolExists === false || typeof obj['undefined'] === 'undefined')

...which can lead to some funky behavior, especially when the obj being tested is a Chai Assertion. See #25

[dvlsg]

I agree, it's definitely odd.

As far as I can tell, global.Symbol.toStringTag only makes it into the scope of index.js after symbolToStringTagExists is calculated (which makes sense).

// test/index.js
// using node v4.3
// index.js utilizing `symbolToStringTagExists`
console.log(Symbol.toStringTag); //=> '__@@toStringTag__'
console.log(Object.prototype.toString.call(extendedArray); //=> '[object ExtendedArray]'
console.log(type(extendedArray)); //=> 'array'

We could technically swap the if statement to be something like this:

// ...
if (
  isArrayExists &&
  Array.isArray(obj) &&
  (typeof Symbol === 'undefined' || typeof Symbol.toStringTag === 'undefined' || typeof obj[Symbol.toStringTag] === 'undefined')
) {
  return 'array';
}

But then we're running those typeof's every time, which seems a bit silly to get this specific case to pass inside the toString sham section for only node 4.3. I'd love to say how much of a performance hit it would be, but my laptop just died on me in the middle of npm run bench. :)

I agree that I don't like the typeof obj['undefined'] stuff, though, for sure. Maybe we're better off trying to make this specific unit test skip v4.3 like v0.10 since Symbol.toStringTag isn't technically available at that point? Idk.

[keithamus]

Typically the precedent is that if we have long conditions, they get assigned to a var which is more readable. For example here:

var symbolToStringTagExists = (symbolExists === false || typeof obj[Symbol.toStringTag] === 'undefined');
if (isArrayExists && Array.isArray(obj) && symbolToStringTagExists)

Would be ideal. Both lines <120 and the if is more readable as the conditions are given names with semantic meaning.

[keithamus]

As far as I can tell, global.Symbol.toStringTag only makes it into the scope of index.js after symbolToStringTagExists is calculated (which makes sense).

Yes... this makes things more complicated. You could sham Symbol.toStringTag before requiring the main lib from inside the tests, which would get around this problem - but it might make other tests fail. I guess give it a try 😄

[meeber]

Now that I think about it, I ran into this same problem when working on #25.

An argument could be made that shamming stuff in the tests is making things more difficult than they need to be. Instead, there could be separate tests depending on if Symbol and/or Symbol.toStringTag are defined, and both situations would be covered via testing multiple node versions in Travis.

[dvlsg]

When I get back to my laptop (hopefully later tonight), I'll try lifting to toStringTag sham stuff toward the top of the file / before the main require, if that's acceptable. If it doesn't affect any of the other unit tests, that might work as a (potentially temporary) fix.

I also like the idea of splitting the tests into separate environments based on Symbol and Symbol.toStringTag availability, but I might stay away from that in this PR (if possible, assuming the toStringTag lifting works) to keep the issues separate.

[dvlsg]

Hah. This is turning into way more trouble than I thought it would be! Here's where it's at:

Lifting the toStringTag sham stuff seems to work as expected, except it only works when I run mocha ./test/index.js directly, because otherwise a different test file will load ./index.js first, which will calculate symbolToStringTag exists before the sham has a chance to run and then cache that require to feed to the other tests. 😞

Considering invalidating the require cache? That would technically work, I think.

[meeber]

Highly recommend that we hold on this PR, create a new PR to refactor tests so that no shamming is involved, then rebase this PR with the changes, thus making writing tests significantly easier (as well as positioning us better for the future).

[keithamus]

Invalidating the require cache in a before step would be a relatively good way to solve this. It's a bit of a cludge but definitely solves the problem.

[dvlsg]

Well, cludge number one (the require reload) had to be wrapped in cludge number two (checking to see if require.resolve() is available) since phantomjs doesn't support it. I suppose we could put together something that doesn't need to use require.resolve() to get the id / path of the module in require.cache, but since the tests seem to work in phantomjs without needing to invalidate the require cache and reload, I opted for this for now.

Also, this commit log is getting fairly noisy, since I keep pushing partial failures. :\ Sounds like we may be holding off on a merge until a test refactor? But it's probably worth squashing to reduce the noise. Github allows squashing before merging, right? Otherwise, I can do it manually.

[keithamus]

Yeah, let's pause progress on this PR for a while, and we'll get the tests reliable in a different PR. If you have some ideas on how to get the tests reliable, please feel free to raise a new PR, otherwise I'll investigate over next weekend.

[keithamus]

@dvlsg if you would like to revisit this issue, I think it'd be much simpler to solve with the latest master. Should just be a case of moving the toStringTag check above Array.isArray.

[keithamus]

Just FYI @dvlsg I'm closing this one, as I've rebased it and made it available at #95, and updated the tests to ensure they're working. Thanks for your hard work on this!

[dvlsg]

Sweet! I got very distracted by other work (apologies!), so I'm glad to see it's making it in! Thank you for picking it up.