d3 / d3-scale

Encodings that map abstract data to visual representation.
https://d3js.org/d3-scale
ISC License
1.59k stars 286 forks source link

adopt InternMap for ordinal scales #237

Closed Fil closed 3 years ago

Fil commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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.