-
Notifications
You must be signed in to change notification settings - Fork 161
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
[geoPath()] Default Values Not Set #66
Comments
That seems like the expected behavior, as the defaults are null. Or are you saying it should return null instead of undefined? I suppose I am being lazy and treating them as equivalent. I’m not sure whether to fix the documentation to say the defaults are undefined, or to coerce the undefined to null… FWIW, it’s also the case that path.projection(undefined) will set the projection to undefined, and not null, and likewise with path.context. |
I think there are a couple of things coming together here. One is pertinent to the API doc in and of itself: The
Which when looking at the corresponding API for the
and correspondingly for the `.context(...) method:
The second aspect relates to the expected usage of folks wanting to use all the goodness of D3 with TypeScript. There have been several asks to tie the definitions down to what is referred to as As of TypeScript 2, there is support for non-nullable types. I.e. before or without the said compiler option set, e.g, The drive behind validating the definitions is obvious, if the definitions reflect the underlying code, and the "contract" is for e.g. Obviously, libraries may in some cases make a semantic distinction between So it's down to as tight as reasonable contracts that avoid unnecessary validation code, while instead relying on the type-safety of the compiler. Apologies for the digression, as I said the validation for IMHO, I would lean towards implementing the defaults as |
Hmm. This issue applies quite a bit more generally than just to d3.geoPath. There are quite a few places in D3 where an accessor expects a function, null or undefined, and will not coerce an undefined value to null. There are also some places that do coerce undefined to null, such as line.context. Generally, I make a minimum effort to validate the input arguments, so the places that replace undefined with null are doing so because they had some reason to check whether the input value was nully. For example, if you pass a nully value to line.context, it means you want to generate an SVG path string rather than render to a CanvasRenderingContext2D, so the line generator checks the specified context and branches the logic as appropriate. It’s tempting to add some logic to D3 to coerce nully input values to null. But it’s more tempting to verify that non nully input values that should be functions are indeed typeof "function", so that an error can be thrown immediately on set rather than on subsequent use with TypeError: value is not a function. There are a few places where I do perform such type checks, such as d3.dispatch, but it does feel like a rabbit hole in the sense that you can never validate the input types perfectly and I don’t feel like I have a good sense in JavaScript of what degree of input validation is appropriate. And yet, the nice thing about TypeScript is that this can be done statically rather than burdening the runtime implementation, right? And so all we really need to do is fix the places where D3 internally violates the type constraint (such as in d3.geoPath where it initializes to undefined rather than null)? |
Apologies for the slight delay in getting back to this. You are right on both counts, for one, the point I raised above regarding That is the primary reason, why I initially deferred it when writing the definitions for the D3 Modules. I went after the low hanging, high value-add fruit first. Nevertheless, yes, the benefit of "validate-once-compile-time-check-many" can be tremendous, where static type-checking is feasible. In particular, since it is possible to use interfaces, generics and function-signatures to set up fairly specific API contracts, where possible without being unnecessarily restrictive. So it's the initial bump we are really talking about (e.g. DefinitelyTyped PR 12566 which is a starting point to refine the d3-selection and the modules most tightly coupled to it). In a large number of cases the D3 API documentation is explicit enough (e.g. the rendering The new As for the broader validation, IMHO the best approach would be to prioritize the modules (in batches as necessary where they are coupled) and set up a mini project:
Such a coordinated effort (spaced out to not overload anyone with cycles they need for other priorities would be great. Given your natural familiarity with the code and, more importantly, understanding of intent and context, it would be great to structure it with your input. Open to suggestions. |
Thanks—this is great feedback. I’m excited that your (and others’) work to support TypeScript will improve D3 even for non-TypeScript users. |
So how about this: we limit this issue specifically to You could open up a thread on At the narrow end of the spectrum is the scrub/validation listed above. At the wider end is the strategic question of bringing the TS definitions into the respective D3 repos, while addressing the narrower needs along the way. For what it's worth, it's about getting the most symbiotic arrangement for the user community and managing the scope/phasing along the way. If you opened a thread up in |
Will update definitions for d3-geo to allow Corresponds to current behavior of module, no further action expected under this issue. |
I’m fixing this to be null as we speak. |
Thanks, I will update the definitions for d3-geo to reflect the changes with 1.4.0. Once you update the D3 Standard Bundle to use d3-geo 1.4, I will bump that as well. |
d3-geo definitions are updated and published. |
I am going over updating the TS definitions for d3-geo 1.3.
With the changes to the
geoPath
API, it seems that when usinggeoPath()
orgeoPath(someProjection)
the defaults are not set internally for omitted arguments.I.e. an omitted project and/or context, will return
undefined
when using the corresponding getter on the returned geo path generator.As I am trying to breeze through the definitions update, you might be faster than me to create a quick PR for this repo. Otherwise, I will circle back to it asap. 😄
The text was updated successfully, but these errors were encountered: