[types-2.0] Add d3-hexbin #12089
[types-2.0] Add d3-hexbin #12089
Conversation
Add https://github.com/d3/d3-hexbin type definitions.
|
d3-hexbin/index.d.ts Checklist
|
There was a problem hiding this comment.
@heavenshell Thanks a lot for contributing this definition to the D3 universe. 😄
Hope you are o.k. with a few changes to consider. They in essence suggest using the same pattern we have applied when writing the other definitions for D3. I.p. to use generics to control type-safety without becoming too restrictive on the one hand or overly permissive by simply using any.
This way, we keep developers on a consistent experience.
| // Definitions by: UNCOVER TRUTH Inc. <https://github.com/uncovertruth/> | ||
| // Definitions: https://github.com/DefinitelyTyped/DefinitelyTyped | ||
|
|
||
| interface Point { |
There was a problem hiding this comment.
There is no need to specify an interface for Point. A generic can and should be used on the generator interface (see below) and the generator factory factory hexbin() (see below as well).
This allows to enforce type-safety throughout and corresponds to the pattern we used in the other D3 module definitions.
There seems to be no need to restrict the generic by setting a extends reference type here. It is truly any, as long as coordinates for x and y can be somehow extracted. This includes the default of [number, number].
As a general note for naming interfaces and data types when writing D3 module definitions, because of the way D3 was conceived, I would recommend not to use very generic names like Point, but rather something like HexbinPoint. This avoids issues with cross-module naming conflicts, when exporting to the same d3 global for vanilla script use scenarios. Same practice we followed elsewhere, e.g. VoronoiPoint.
| interface Point { | ||
| [id: string]: number | string | ||
| } | ||
|
|
There was a problem hiding this comment.
There should be an additional interface for the bin(s) returned by the generator, when applied to the points.
I.e. something like:
export interface HexbinBin<T> extend
10000
s Array<T> {
x: number;
y: number;
}This corresponds to the data type of an element in bin array returned by the applied generator, as per API
Each bin in the returned array is an array containing the bin’s points. Only non-empty bins are returned; empty bins without points are not included in the returned array. Each bin has these additional properties:
x - the x-coordinate of the center of the associated bin’s hexagon y - the y-coordinate of the center of the associated bin’s hexagonThese x- and y-coordinates of the hexagon center can be used to render the hexagon at the appropriate location in conjunction with hexbin.hexagon.
| [id: string]: number | string | ||
| } | ||
|
|
||
| export interface Hexbin { |
There was a problem hiding this comment.
The interface for the generator should be generisized to keep it general, but typesafe:
export interface Hexbin<T> {
...
}Here T corresponds to the type of a data point, which can be general, as long as the developer specifies the x- and y-accessors to extract the respective coordinates.
The defaults would correspond to T : [number, number], but it could be anything like
interface CrazyT {
point: {
x: number,
y: number
};
moreStuff: any;
}| * If either the x- or y-coordinate is NaN, the point is ignored and will | ||
| * not be in any of the returned bins. | ||
| */ | ||
| (hexbin: Array<[number, number]> | Array<Point>): any; |
There was a problem hiding this comment.
The function signature should be:
(points: T[]): HexbinBin<T>[]
I.e. the generic controls the input and the output of the returned bins array.
| * of each point. The default value assumes each point is specified as | ||
| * a two-element array of numbers [x, y]. | ||
| */ | ||
| x(_: any): Hexbin; |
There was a problem hiding this comment.
This signature should be x(x: (d: T) => number): Hexbin<T>
This will ensure that the accessor function is type-safe with respect to the data type of the points being passed in, and returning a number.
| * of each point. The default value assumes each point is specified as | ||
| * a two-element array of numbers [x, y]. | ||
| */ | ||
| y(): (d?: any) => number; |
There was a problem hiding this comment.
This should be y(): (d: T) => number; for type safety.
| 8000 | * the width of each hexagon is radius × 2 × sin(π / 3) | |
| * and the height of each hexagon radius × 3 / 2. | ||
| */ | ||
| radius(_: any): Hexbin; |
There was a problem hiding this comment.
This should be more restrictive: radius(radius: number): Hexbin<T>.
| * If extent is specified, sets the hexbin generator’s extent to the | ||
| * specified bounds [[x0, y0], [x1, y1]] and returns the hexbin generator. | ||
| */ | ||
| extent(_: any): Hexbin; |
There was a problem hiding this comment.
This should be more restrictive: extent(extent: [[number, number], [number, number]]): Hexbin<T>;
| * | ||
| * The extent defaults to [[0, 0], [1, 1]]. | ||
| */ | ||
| extent(): Array<[number, number]>; |
There was a problem hiding this comment.
Consider using [[number, number], [number, number]] as the return type. This is what we have done everywhere else in D3 module definitions with extent.
| /** | ||
| * Constructs a new default hexbin generator. | ||
| */ | ||
| export function hexbin(): Hexbin; |
There was a problem hiding this comment.
Consider using two signatures for the generator factory:
export function hexbin(): Hexbin<[number, number]>;
export function hexbin<T>(): Hexbin<T>;The first one corresponds to the default behavior, where the x- and y-accessor expect a point to be passed in directly as coordinates.
The second allows the developed to control the generic type which is enforced throughout the Hexbin<T> generator and all its accessors and return types, like the HexbinBin<T>[].
|
@tomwanzek |
|
@heavenshell no worries, if you take a look at the interface for I.e. it defines a generator interface with accessors for x- and y-coordinates. And it has a few overloaded factories to create a generator. There are some more than you would need, as the factory can take arguments. If you have any other questions, don't hesitate. Cheers. T |
|
@tomwanzek These tests are ported from https://github.com/d3/d3-hexbin/blob/master/test/hexbin-test.js I'm really appreciated if you give me advices to help solove problems. P.S. |
|
@heavenshell With a bit of luck, I can fit it in some time later today. Latest on the weekend. Could you push a commit with the proposed changes to the definitions file to this PR. This should make it easier to use the review feature to comment on specific lines, if need be. As for tests, bear mind that one of the design choices of DefinitelyTyped was that, tests are essentially shape tests only. I.e. they only validate that the test file and its related definitions compile without error, they do not emit or execute anything beyond that. So testing the shape is essentially typing the left and right-side of an assignment independently and checking that the assignment does not fail, same with arguments to functions/methods and their return values. |
Use generics to control type-safety.
|
@tomwanzek
Yeah, right. Sorry I'm not a good at English. If you were confused it's so sorry 😢 |
|
Thanks for pushing the changes, I will look at it later. As for language, no concerns there... my Japanese is distinctly worse. Which is to say, non-existent. Now that is really sad 😉 |
|
I had a look at the definitions, and structurally they appear accurate. So it's really just down to completing the tests. I am copying in a short snippet here that should shed some light on how to apply the definitions in the tests. Clearly, when writing "production code", some of the explicit typing of variables below might be just inferred from an assignment. However, this allows testing the shape using a compile only test. The key to using the factory, the generator and the accessors, is illustrated by the use of the import * as d3Hexbin from 'd3-hexbin';
interface Point {
x0: number;
y0: number;
}
let hb: d3Hexbin.Hexbin<Point>;
let bins: d3Hexbin.HexbinBin<Point>[];
// Create generator =======================================
hb = d3Hexbin.hexbin<Point>();
// Configure generator =====================================
// x Accessor ----------------------------------------------
let x: (d:Point) => number;
x = function (d: Point) { return d.x0; };
// test setter
hb = hb.x(x);
// test getter
x = hb.x();
// y Accessor ----------------------------------------------
let y: (d:Point) => number;
y = function (d: Point) { return d.y0; };
// test setter
hb = hb.y(y);
// test getter
y = hb.y();
// Use generator ============================================
bins = hb([
{ x0: 0, y0: 0 }, { x0: 0, y0: 1 }, { x0: 0, y0: 2 },
{ x0: 1, y0: 0 }, { x0: 1, y0: 1 }, { x0: 1, y0: 2 },
{ x0: 2, y0: 0 }, { x0: 2, y0: 1 }, { x0: 2, y0: 2 }
]);
interface RemappedBin {
binCoordinates: [number, number];
points: Point[];
}
let remappedBins: Array<RemappedBin>;
remappedBins = bins.map(bin => {
const x: number = bin.x; // x-coordinate of bin
const y: number = bin.y; // y-coordinate of bin
const pointsInBin: Point[] = bin.map(p => {
const point: Point = p;
return point;
});
const remapped: RemappedBin = {
binCoordinates: [x, y],
points: pointsInBin
};
return remapped;
});EDIT: Updated code-snippet for |
| @@ -51,7 +52,7 @@ export interface Hexbin { | |||
| * of each point. The default value assumes each point is specified as | |||
| * a two-element array of numbers [x, y]. | |||
| */ | |||
| x(_: any): Hexbin; | |||
| x(_: (d: T) => number): Hexbin<T>; | |||
There was a problem hiding this comment.
Minor edit: change _ to x.
|
@tomwanzek |
|
Hey, whenever you're ready. Great that you picked up contributing d3-hexbin! T |
|
@tomwanzek |
|
Not sure, if I get to it today. But I will try asap. Thx, T |
eae5d89 to
ec9eb4b
Compare
|
@tomwanzek @heavenshell I really don't like the structure, it doesn't feel right that in typescript I need to use it as It feels a lot better to override or merge the d3 namespace, but I don't know how. I tried a few approaches without success. Have you tried this approach also? Or you think that what you have defined is the best approach? |
|
@heavenshell I have been working away on moving the initial batches of "core" D3 module definitions to support I had not checked this one for @herkulano Is your question on how to use it related to |
|
@tomwanzek thanks for coming back to me. My question is not on how to use it, but rather on how to create the It's clear to me how it can be used in "(1) using your plug-in with a module loader (i.e. using import)". In the case of the import { hist2d } from 'd3-hist2d';
hist2d().bins(10)
// OR
import * as h2 from 'd3-hist2d';
h2.hist2d().bins(10)My point is that this feels wrong. With d3 v4 all parts of d3 are modules now, so third-party modules should be the same as d3 default modules, this is true in Javascript so it should also be true in Typescript. It's weird having to change the syntax to use it. In the definition files we should override/merge the So then overriding/merging the d3 namespace we could then use any plugin like this, or something similar to this: import 'd3-hist2d';
d3.hist2d().bins(10) |
|
There is a simple reason, why I asked the question regarding how you would like to use the plug-in. Scenario 1: As import with module loader
When used as proper modules, all D3 modules are anonymous, so your module definitions already correspond to the way all other definitions are written. There is no magic glue // d3-with-hist2d-bundle.ts
// re-export all the "standard" d3-modules you require
// or simply use export * from 'd3' for the entire standard D3 bundle
export * from 'd3-array';
export * from 'd3-selection';
...
export * from 'd3-hist2d';Then if you want to use it as a more "conventional" d3 object simply import from this barrel: // whatever-module-imports-d3.ts
import * as d3 from './d3-with-hist2d-bundle';
let x = d3.hist2d(); // et voilaScenario 2: As plain vanilla script This is where you actually run into a typescript vs D3 restriction. D3 code was written in a way, that the created UMD modules all merge into the same Assuming, you are writing your module the same way and using Rollup the same way, your JS code will support the same behavior as the other D3 scripts. Important aside: As a word of caution, avoid clashing with variables/methods/interface names already defined for d3 modules (or their corresponding definition files w.r.t. TypeScript.) In principle, creating a definition file for a UMD module, can be accomplished by writing the definitions for the proper module and adding: export as namespace d3;The definition file for the standard D3 bundle actually does that. The definition files for the individual modules do not. This is due to TypeScript Issue 9681. The full history of this discussion is also embedded in #9936. The resolution to this problem can only be achieved with a workaround at the level of the project using the definitions. Add a directory, e.g. In the // index.d.ts
export as namespace d3;
// re-export all the "standard" d3-modules you require
export * from 'd3-array';
export * from 'd3-selection';
...
export * from 'd3-hist2d';Make sure you add the This way, should you write a plain vanilla script in typescript, you should have access to the merged |
|
@tomwanzek thank you so much for your patience and thorough answer, really appreciate it! 💯 |
|
@herkulano You're welcome, one more module in the ecosystem due to your work, so props to you! |
| "module": "commonjs", | ||
| "target": "es6", | ||
| "noImplicitAny": true, | ||
| "strictNullChecks": false, |
There was a problem hiding this comment.
Use "strictNullChecks": true, based on a quick validation, your definitions should already be valid with this option enabled.
We have just started the complete validation/updating of the core D3 modules. So you are ahead of the curve.
There was a problem hiding this comment.
OK, I'll update to strictNullChecks: true
| @@ -0,0 +1,138 @@ | |||
| // Type definitions for D3JS d3-hexbin module v0.2.1 | |||
There was a problem hiding this comment.
Use the following first line for the header, as the tslint rules have recently become stricter on the header comments:
// Type definitions for D3JS d3-hexbin module 0.2The "v" and the patch release number are no longer permissible.
There was a problem hiding this comment.
OK, I'll fix it.
| // Project: https://github.com/d3/d3-hexbin/ | ||
| // Definitions by: UNCOVER TRUTH Inc. <https://github.com/uncovertruth/>, Tom Wanzek <https://github.com/tomwanzek> | ||
| // Definitions: https://github.com/DefinitelyTyped/DefinitelyTyped | ||
|
|
There was a problem hiding this comment.
This one is related to the changes to the first line of the header comments. It is purely optional and simply a convention I personally follow. After an empty line below the main header, I add in a comment indicating the full version number of the latest release I validated the definition against. I find it easier as a checkpoint when looking for new changes in the underlying module. This way I do not need to compare commit dates. I use the following comment:
// Last module patch version validated against: 0.2.1
There was a problem hiding this comment.
OK, I'll add your comment.
// Last module patch version validated against: 0.2.1
| * used to determine which hexagonal bin to add the point. | ||
| * If either the x- or y-coordinate is NaN, the point is ignored and will | ||
| * not be in any of the returned bins. | ||
| */ |
There was a problem hiding this comment.
Add to JSDoc comments:
* @param points An array of data points to bin.
There was a problem hiding this comment.
OK, I'll fix it.
| * If radius is specified, a hexagon with the specified radius is returned; | ||
| * this is useful for area-encoded bivariate hexbins. | ||
| * | ||
| * @param {number} radius Radius number |
There was a problem hiding this comment.
I tend not to specify the argument type, e.g. number here, in the @param statements within TypeScript/JSDoc. The main reason is ease of re-factoring. It's easily overlooked when using IDE functionality meant for refactoring, which does not touch the type in the comments.
When an argument is optional, I tend to briefly indicated it in the @param description, e.g. `An optional radius parameter to be used when generating the hexagon.
There was a problem hiding this comment.
OK, remove {number}.
@param radis An optional radius parameter to be used when generating the hexagon
is ok?
| // Definitions: https://github.com/DefinitelyTyped/DefinitelyTyped | ||
|
|
||
| export interface HexbinBin<T> extends Array<T> { | ||
| x: number; |
There was a problem hiding this comment.
Explanatory comment for x:
/*
* x-coordinate of the center of the associated bin’s hexagon.
*/
There was a problem hiding this comment.
OK, I'll fix it.
|
|
||
| export interface HexbinBin<T> extends Array<T> { | ||
| x: number; | ||
| y: number; |
There was a problem hiding this comment.
Explanatory comment for y:
/*
* y-coordinate of the center of the associated bin’s hexagon.
*/
There was a problem hiding this comment.
OK, I'll fix it.
| y: number; | ||
| } | ||
|
|
||
| export interface Hexbin<T> { |
There was a problem hiding this comment.
JSDoc comment for interface itself:
/**
* A generator for hexagonal binning.
*
* Hexagonal binning is useful for aggregating “big” data into a coarse, “small” representation for display:
* rather than render a scatterplot of tens of thousands of points, bin the points into hexagons and show the distribution using color or area.
*
* The generic corresponds to the data type of an element in the points array.
*/There was a problem hiding this comment.
OK, I'll fix it.
|
|
||
| { | ||
| // hexbin.hexagon(radius) uses the current bin radius if radius is null | ||
| let path: string = d3Hexbin.hexbin().hexagon(null); |
There was a problem hiding this comment.
Passing in null as the radius should now fail with strictNullChecks on. The definitions only allow number | undefined, radius being optional. Here there is no need to support null in the definitions, as it has no special meaning, like "removing a listener or accessor function and returning to default behavior".
There was a problem hiding this comment.
fmm, so do I need to remove this test?
| @@ -0,0 +1,211 @@ | |||
| /** | |||
There was a problem hiding this comment.
As a general note, since the tests in DT are strictly "shape tests", there are a few patterns I tend to follow in the other D3 module definitions:
- When defining variables/constants always explicitly type them with the expected type. I.e.
let x : number = ...rather thanlet x = ...This way, tests will fail if the right-hand side does not meet the expectation. Otherwise, the definitions could appear internally consistent due to TypeScripts implicit type inference. The same holds for accessor functions to be tested as the return type of a getter. - Test each signature for each interface method
- For a "setter" method signature always test the return type individually. Do not use consecutive chaining in shape tests. Otherwise, the test may not find edge cases where a return type is (accidentally) changed by one method, but the next chained method is also defined on it, e.g. a sub-type. Re-assigning to the object on which a setter was invoked is a quick check for chainability.
- As in the case of
hexbin(), there are sometimes generator factories, which make a default assumption about underlying data types (like each point is of type[number, number]), but allow for more general data structures. In this case by setting the genericTe.g. to typePoint. Again, I would first define a variable with explicit type, likeHexbin<[number, number]>orHexbin<Point>, then test the factory by assigning to those variables. - When there are accessors involved, like here for
xandy, I tend to test against each of the typed generators I obtained from the factory.
One aspect, that is helpful is creating explicit tests for expected failure modes, e.g. assigning let x: d3Hexbin.Hexbin<Point> = d3Hexbin.hexbin();. Same for e.g. method signatures with accessor functions depending on a generic. The benefit is that it may catch some edge cases. Note that such tests can only be inspected by you when updating tests. As you should see compiler errors in your IDE right away. You will need to comment them out before committing, so the CI tests do not fail. I tend to do that by also appending a comment like // fails, argument type mismatch or something similar.
You can find examples of the latter e.g. in d3-selection.
|
@heavenshell could you respond to the review comments? |
|
@tomwanzek Merging this, you can make the changes you mentioned if you want to. |
|
sorry for late response. |
|
@heavenshell Great! Please do that in a new pull request, though, and based on the |
|
@Andy-MS |
|
Will do in a spare minute. Thx for the contribution and merge. |
case 1. Add a new type definition.
--target es6and--noImplicitAnyoptions.-tests.tsor-tests.tsx.d3-hexbinis not exists. So we add type definition.