adopt InternMap for ordinal scales

[Fil]

No description provided.

[curran]

I notice that when I run npm test on this branch, there are 6 failing tests.

$ npm test |grep "not ok"

src/index.js → dist/d3-scale.js...
created dist/d3-scale.js in 286ms

src/index.js → dist/d3-scale.min.js...
created dist/d3-scale.min.js in 1.2s
not ok 699 should be equal
not ok 702 should be equal
not ok 703 should be equal
not ok 719 should be equivalent
not ok 721 should be equal
not ok 722 should be equal

[Fil]

Yes it fails because keys were coerced to strings, but now we can distinguish 0 and "0"—which risks to break a lot of applications.

A solution is to keep coercing everything to string when looking in the index:

diff --git a/src/ordinal.js b/src/ordinal.js
index f4513c5..0a34644 100644
--- a/src/ordinal.js
+++ b/src/ordinal.js
@@ -10,19 +10,21 @@ export default function ordinal() {
       unknown = implicit;

   function scale(d) {
-    if (!index.has(d)) {
+    const key = "" + d;
+    if (!index.has(key)) {
       if (unknown !== implicit) return unknown;
-      index.set(d, domain.push(d));
+      index.set(key, domain.push(d));
     }
-    return range[(index.get(d) - 1) % range.length];
+    return range[(index.get(key) - 1) % range.length];
   }

   scale.domain = function(_) {
     if (!arguments.length) return domain.slice();
     domain = [], index = new InternMap();
     for (const value of _) {
-      if (index.has(value)) continue;
-      index.set(value, domain.push(value));
+      const key = "" + value;
+      if (index.has(key)) continue;
+      index.set(key, domain.push(value));
     }
     return scale;
   };

This fixes the tests, but then it's not better than the current implementation…

[curran]

now we can distinguish 0 and "0"—which risks to break a lot of applications.

This feels like a highly desirable breaking change that merits a major version bump on d3-scale.

[mbostock]

We could pass String as the second argument to the InternMap constructor, but I think the whole goal here is to rely on valueOf instead of string coercion. Making it a major version bump sounds fine to me, although it’ll be a pain to update hundreds of notebooks for such a tiny change.