fix: account for sorting bug in node 10 #205
Conversation
Codecov Report
@@ Coverage Diff @@
## master #205 +/- ##
==========================================
+ Coverage 97.1% 97.14% +0.04%
==========================================
Files 1 1
Lines 138 140 +2
Branches 35 35
==========================================
+ Hits 134 136 +2
Misses 4 4
Continue to review full report at Codecov.
|
|
@kentcdodds @Gpx found a pretty little stank nasty in node 10 and prior. code cov is off by a branch but it's 100% if you count the fact that it's tested in each env on CI. ;) update: cool looks like codecov accounts for that, neat. |
src/index.js
Outdated
| @@ -12,6 +12,23 @@ function findTagInParents(element, tagName) { | |||
| return findTagInParents(element.parentNode, tagName); | |||
| } | |||
|
|
|||
| let sortListByTabIndex; | |||
| let nodeVersion = +process.version.slice(1).split('.')[0]; | |||
| if (nodeVersion < 11) { | |||
There was a problem hiding this comment.
Is there any way we can make this consistent regardless of node version? I'm not super thrilled about the added complexity here 😬
There was a problem hiding this comment.
Yeah I thought about that but it would mean an additional map over the entire list. Felt like it would be punishing node 12 for the sins of node 10. Also didn't think it warranted adding a separate sort just because of this. As always open to other options I might be missing
There was a problem hiding this comment.
I don't think we should be worried about perf at this stage... We're paying the price of complexity for an unnoticeable perf gain 🤷♂️
There was a problem hiding this comment.
alrighty. pushed a version that just has a single sort @kentcdodds
|
🎉 This PR is included in version 8.0.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
prior to node 11, Array.sort was unstable for arrays w/ length > 10.
https://twitter.com/mathias/status/1036626116654637057
for this reason, the tab() function needs to account for this in it's sorting.
for example under node 10 in this test: