search

tomwanzek / d3-v4-definitelytyped

[DEPRECATED] This repo was intended as a staging area for typescript definitions supporting the latest major release of D3js (i.e. version 4.1.x) by Mike Bostock. It has been migrated to DefinitelyTyped.
MIT License
53 stars 14 forks source link

d3-geo Suggestions #95

Closed tomwanzek closed 8 years ago

tomwanzek commented 8 years ago

@Ledragon I took a longer look at d3-geo (including the source code, some of the old 3.x API documentation, and played around with d3-geo in the browser console a bit.)

Based on that I put together this PR for your review. Let me know what you think. As I said before, this one really was not as easy as some of the others purely off the API doc. I tried to comment out all my change suggestions in the individual commit messages.

Ledragon commented 8 years ago

I don't even know what to say ___ This is brilliant work, it is so complete. So green lights to merge the PR of course.

For what I see, maybe some contributing rules could be added, such as creating one line per method overload rather than optional/multiple types in a single method.

Also, based on what you found and completed, might it be worth augmenting the actual d3-geo documentation?

tomwanzek commented 8 years ago

@Ledragon Thanks could not have done it without your collaboration...

As for updating the d3-geo documentation together with Mike, I think that would be great for everyone. There are a couple of slightly open question marks I was left with when preparing this PR:

  1. It seems streams do not have to implement the sphere() method. Or at least, e.g. geoClipExtent() does not implement it when returning the wrapped stream. The same is true when using geoPath() without having passed in a well-defined (custom) projection. This creates the little predicament, that passing { type="Sphere" } or a collection containing it into geoPath() will create a runtime error. This item alone is worth a follow-up with Mike regarding what the expected behavior is.
  2. Strictly speaking the return type of geoTransform(prototype) is { stream: (s: GeoStream) => (T & GeoStream & GeoStreamWrapper)} when seeing what the code does or testing in the debugger console. I dropped & GeoStreamWrapper already, as I consider the wrapped initial stream a private property private. Arguably, the same argument could be made for the T & properties inI excess of what GeoStream provides. They could be considered private to the wrapper as well, which would mean the return type of geoTransform(...) is simply { stream: (s: GeoStream) => GeoStream }, except with the internalized transformation kept private.

Wrt to separating the overloaded signatures, I think you are right, ideally the style guide should be a bit more explicit in this regard. If I have time after the completing the PRs currently in the pipeline here and on DT, I may add on a little.

On that note, the full generic mouseover help for geoPath() is a beauty in VSCode, given the flexibility Mike has baked into what it can handle :smile: