d3 / d3-color

Color spaces! RGB, HSL, Cubehelix, CIELAB, and more.
https://d3js.org/d3-color
ISC License
398 stars 91 forks source link

remove Map and map polyfill, change d3-bundler dep version #16

Closed boygirl closed 8 years ago

boygirl commented 8 years ago

cc/ @mbostock

This PR removes Map and the Map polyfill in favor of plain objects. We're having issues getting the polyfill to work, and it seems like a lot of overhead for not every much gain.

Thanks! FormidableLabs

mbostock commented 8 years ago

This doesn’t behave correctly when passed a color specification that conflicts with a JavaScript built-in property, such as __proto__ or hasOwnProperty.

Also, I would prefer to write code in a forward-looking manner that starts to take advantage of some of the more widely-available parts of ES6 (including Map and Set).

Could you elaborate on the issues the polyfill was causing?

mbostock commented 8 years ago

(See the old d3.map class in D3 3.x for how this was previously implemented. I’d just rather be writing code for the future than writing kludgy code against objects. Though… I agree that since the keys are always strings I may be too aggressively futuristic.)

boygirl commented 8 years ago

cc/ @mbostock I'm all for bleeding edge JS, and our work is also in es6. The Map polyfill included by d3-bundler is not working with our phantomjs test infrastructure, and we think not being included in our builds. Unfortunately we can't transpile Map easily into es5, so our options are, including the babel runtime (or something like it) which adds a lot of weight, including a Map polyfill of our own which some infrastructure / process gurus in our midst are philosophically opposed to, or forking/PRing the polyfilled repo.

mbostock commented 8 years ago

Right. The Map “polyfill” provided by d3-bundler isn’t really a polyfill for ES6 maps, but just for the limited subset that D3 uses—most significantly, with the restriction that the keys are always strings.

But, I still don’t understand why the provided polyfill wouldn’t work with your PhantomJS test infrastructure or builds. If you can explain why that’s not working, then hopefully I can suggest a forward-looking solution that meets both our needs.

boygirl commented 8 years ago

This is the error I'm seeing. This doesn't really explain why it's not working.

PhantomJS 1.9.8 (Mac OS X 0.0.0) ERROR
  ReferenceError: Can't find variable: Map

I looked at the polyfill code in the d3-bundler repo, and nothing stuck out as problematic there. My only guesses are that the d3-bundler is not correctly including the polyfill, or there is some kind of mismatch between how you are building this project, and how we are trying to consume/build it (webpack).

mbostock commented 8 years ago

Did you write your own build process to convert the ES6 modules to ES5, or are you using the pre-built UMD (as build/color.js or build/color.min.js)?

boygirl commented 8 years ago

We're including packages via npm, so according to the package.json that should be the pre-built version. We are using babel to convert our own es6 to es5, but we are not running any external packages through that step of our build process.

mbostock commented 8 years ago

Okay. I think it makes sense to abandon the Map polyfill approach, largely because it’s not a true polyfill since it only supports string keys and the API is different. I haven’t decided yet if it’s worth releasing a new d3-map module to serve the purpose of d3.map in 4.0, but it probably is, since using plain objects is error prone.