Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

boygirl
Copy link

@boygirl boygirl commented Aug 11, 2015

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
Copy link
Member

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
Copy link
Member

(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
Copy link
Author

boygirl commented Aug 11, 2015

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
Copy link
Member

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
Copy link
Author

boygirl commented Aug 11, 2015

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
Copy link
Member

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
Copy link
Author

boygirl commented Aug 12, 2015

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 added a commit that referenced this pull request Sep 15, 2015
@mbostock
Copy link
Member

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.

@mbostock mbostock closed this Sep 15, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants