Skip to content

[d3-axis and d3-format] activate strictNullChecks and strictFunctionTypes #24305

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.

Sign up for GitHub

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

Closed
denisname wants to merge 10 commits into from
Closed

[d3-axis and d3-format] activate strictNullChecks and strictFunctionTypes #24305

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
21299f9
Activate strictNullChecks in d3-format
denisname Mar 14, 2018
a97b6fe
Use 'number coercible' type in d3-format
denisname Mar 14, 2018
12461ad
Add @throws and fix small typos in comments
denisname Mar 14, 2018
ee100da
Specify numerals type
denisname Mar 14, 2018
6c309cd
Remove 'no-unnecessary-generics: false' for d3-axis
denisname Mar 14, 2018
900c476
Use $ExpectError for failing tests
denisname Mar 14, 2018
278b13f
Activate strictFunctionTypes in d3-axis
denisname Mar 14, 2018
e164c7d
Fix small typos in d3-axis
denisname Mar 15, 2018
559a1f7
Update d3-format
denisname Mar 22, 2018
411b81f
Scale is generic
denisname Mar 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 23 additions & 7 deletions types/d3-axis/d3-axis-tests.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ axisScaleNumber = scaleBand<number>();
axisScaleNumber = scalePoint<number>();
axisScaleString = scaleBand();
axisScaleString = scalePoint();

// --------------------------------------------------------------------------
// Test AxisContainerElement
// --------------------------------------------------------------------------
Expand All @@ -58,7 +59,8 @@ const canvas: HTMLCanvasElement = select<HTMLCanvasElement, any>('canvas').node(

containerElement = svg;
containerElement = g;
// containerElement = canvas; // fails, incompatible type
// $ExpectError
Copy link
Copy Markdown
Contributor

@tomwanzek tomwanzek Mar 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for starting to use this dts-lint feature. We should start using this across the board on all the "fail" tests in D3. We had to historically comment them out (and manually check during definition updates). So let's roll with $ExpectError

containerElement = canvas; // fails, incompatible type

// --------------------------------------------------------------------------
// Test Axis Generators
Expand All @@ -77,14 +79,17 @@ let leftAxis: d3Axis.Axis<number | { valueOf(): number }> = d3Axis.axisLeft(scal

leftAxis = leftAxis.scale(scalePow());
const powerScale: ScalePower<number, number> = leftAxis.scale<ScalePower<number, number>>();
// powerScale = leftAxis.scale(); // fails, without casting as AxisScale is purposely generic
// $ExpectError
const powerScale2 = leftAxis.scale(); // fails, without casting as AxisScale is purposely generic

bottomAxis = bottomAxis.scale(scaleOrdinal<number>());
// bottomAxis = bottomAxis.scale(scalePow()) // fails, domain of scale incompatible with domain of axis
// $ExpectError
bottomAxis = bottomAxis.scale(scalePow()); // fails, domain of scale incompatible with domain of axis

const axisScale: d3Axis.AxisScale<string> = bottomAxis.scale();
const ordinalScale: ScaleOrdinal<string, number> = bottomAxis.scale<ScaleOrdinal<string, number>>();
// ordinalScale = bottomAxis.scale(); // fails, without casting as AxisScale is purposely generic
// $ExpectError
const ordinalScale2 = bottomAxis.scale(); // fails, without casting as AxisScale is purposely generic

// ticks(...) ----------------------------------------------------------------

Expand Down Expand Up @@ -119,6 +124,7 @@ const formatFn: ((domainValue: string, index: number) => string) | null = bottom

bottomAxis.tickFormat((d, i) => '#' + i);
bottomAxis.tickFormat(d => d + '!');

// tickSize(...) ----------------------------------------------------------------

rightAxis = rightAxis.tickSize(5);
Expand Down Expand Up @@ -149,14 +155,24 @@ const gTransition = gSelection.transition();
gSelection.call(topAxis);
gTransition.call(topAxis);

const svgSelection: Selection<SVGSVGElement, any, any, any> = select<SVGSVGElement, any>('g');
const svgSelection: Selection<SVGSVGElement, any, any, any> = select<SVGSVGElement, any>('svg');
Copy link
Copy Markdown
Contributor

@tomwanzek tomwanzek Mar 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch 😄 Now that's attention to detail!

const svgTransition = svgSelection.transition();

svgSelection.call(leftAxis);
svgTransition.call(leftAxis);

const pathSelection: Selection<SVGPathElement, any, any, any> = select<SVGPathElement, any>('path');
const pathTransition = svgSelection.transition();

// $ExpectError
pathSelection.call(bottomAxis);
// $ExpectError
pathSelection.call(bottomAxis);

const canvasSelection: Selection<HTMLCanvasElement, any, any, any> = select<HTMLCanvasElement, any>('canvas');
const canvasTransition = canvasSelection.transition();

// canvasSelection.call(rightAxis); // fails, incompatible context container element
// canvasTransition.call(rightAxis); // fails, incompatible context container element
// $ExpectError
canvasSelection.call(rightAxis); // fails, incompatible context container element
// $ExpectError
canvasTransition.call(rightAxis); // fails, incompatible context container element
30 changes: 15 additions & 15 deletions types/d3-axis/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,43 +29,43 @@ export interface AxisTimeInterval {

/**
* A helper interface to which a scale passed into axis must conform (at a minimum)
* for axis to use the scale without error
* for axis to use the scale without error.
*/
export interface AxisScale<Domain> {
(x: Domain): number | undefined;
domain(): Domain[];
range(): number[];
copy(): this;
bandwidth?(): number;
// TODO: Reconsider the below, note that the compiler does not differentiate the overloads w.r.t. optionality
// TODO: Reconsider the below, note that the compiler does not differentiate the overloads w.r.t. optionality
// ticks?(count?: number): Domain[];
// ticks?(count?: AxisTimeInterval): Date[];
// tickFormat?(count?: number, specifier?: string): ((d: number) => string);
// tickFormat?(count?: number | AxisTimeInterval, specifier?: string): ((d: Date) => string);
}

/**
* A helper type to alias elements which can serve as a container for an axis
* A helper type to alias elements which can serve as a container for an axis.
*/
export type AxisContainerElement = SVGSVGElement | SVGGElement;

/**
* Interface defining an axis generator. The generic <Domain> is the type of the axis domain
* Interface defining an axis generator. The generic <Domain> is the type of the axis domain.
*/
export interface Axis<Domain> {
Copy link
Copy Markdown
Contributor

@tomwanzek tomwanzek Mar 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will review the constraints on Domain in the definition for Axis and AxisScale to define a suitable default for the generic parameters (TS 2.3 feature that we should use here as well). We might be able to do better than any for the most general permissible case.

We could have export interface Axis<Domain extends DomainConstraint = DomainDefault> {...} and export interface AxisScale<Domain extends DomainConstraint = DomainDefault> {...}. Then in day-to-day use one could just do let x: Axis; for brevity, if the defaults are good enough.

/**
* Render the axis to the given context.
*
* @param context A selection of SVG containers (either SVG or G elements).
*/
(context: Selection<AxisContainerElement, any, any, any>): void;
(context: Selection<SVGSVGElement, any, any, any> | Selection<SVGGElement, any, any, any>): void;

/**
* Render the axis to the given context.
*
* @param context A transition defined on SVG containers (either SVG or G elements).
*/
(context: TransitionLike<AxisContainerElement, any>): void;
(context: TransitionLike<SVGSVGElement, any> | TransitionLike<SVGGElement, any>): void;

/**
* Gets the current scale underlying the axis.
Expand All @@ -75,7 +75,7 @@ export interface Axis<Domain> {
/**
* Sets the scale and returns the axis.
*
* @param scale The scale to be used for axis generation
* @param scale The scale to be used for axis generation.
*/
scale(scale: AxisScale<Domain>): this;

Expand All @@ -86,7 +86,7 @@ export interface Axis<Domain> {
*
* This method is also a convenience function for axis.tickArguments.
*
* @param count Number of ticks that should be rendered
* @param count Number of ticks that should be rendered.
* @param specifier An optional format specifier to customize how the tick values are formatted.
*/
ticks(count: number, specifier?: string): this;
Expand Down Expand Up @@ -178,7 +178,7 @@ export interface Axis<Domain> {
*
* See also axis.ticks.
*
* @param args An array with arguments suitable for the scale to be used for tick generation
* @param args An array with arguments suitable for the scale to be used for tick generation.
*/
tickArguments(args: any[]): this;

Expand Down Expand Up @@ -210,7 +210,7 @@ export interface Axis<Domain> {
tickFormat(): ((domainValue: Domain, index: number) => string) | null;

/**
* Sets the tick format function and returns the axis.
* Sets the tick format function and returns the axis.
*
* @param format A function mapping a value from the axis Domain to a formatted string
* for display purposes. When invoked, the format function is also passed a second argument representing the zero-based index
Expand Down Expand Up @@ -287,7 +287,7 @@ export interface Axis<Domain> {
/**
* Set the current padding and return the axis.
*
* @param padding Padding in pixels (Default is 3).
* @param padding Padding in pixels (Default is 3).
*/
tickPadding(padding: number): this;
}
Expand All @@ -296,30 +296,30 @@ export interface Axis<Domain> {
* Constructs a new top-oriented axis generator for the given scale, with empty tick arguments,
* a tick size of 6 and padding of 3. In this orientation, ticks are drawn above the horizontal domain path.
*
* @param scale The scale to be used for axis generation
* @param scale The scale to be used for axis generation.
*/
export function axisTop<Domain>(scale: AxisScale<Domain>): Axis<Domain>;

/**
* Constructs a new right-oriented axis generator for the given scale, with empty tick arguments,
* a tick size of 6 and padding of 3. In this orientation, ticks are drawn to the right of the vertical domain path.
*
* @param scale The scale to be used for axis generation
* @param scale The scale to be used for axis generation.
*/
export function axisRight<Domain>(scale: AxisScale<Domain>): Axis<Domain>;

/**
* Constructs a new bottom-oriented axis generator for the given scale, with empty tick arguments,
* a tick size of 6 and padding of 3. In this orientation, ticks are drawn below the horizontal domain path.
*
* @param scale The scale to be used for axis generation
* @param scale The scale to be used for axis generation.
*/
export function axisBottom<Domain>(scale: AxisScale<Domain>): Axis<Domain>;

/**
* Constructs a new left-oriented axis generator for the given scale, with empty tick arguments,
* a tick size of 6 and padding of 3. In this orientation, ticks are drawn to the left of the vertical domain path.
*
* @param scale The scale to be used for axis generation
* @param scale The scale to be used for axis generation.
*/
export function axisLeft<Domain>(scale: AxisScale<Domain>): Axis<Domain>;
2 changes: 1 addition & 1 deletion types/d3-axis/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
"noImplicitAny": true,
"noImplicitThis": true,
"strictNullChecks": true,
"strictFunctionTypes": false,
"strictFunctionTypes": true,
"baseUrl": "../",
"typeRoots": [
"../"
Expand Down
21 changes: 20 additions & 1 deletion types/d3-format/d3-format-tests.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,19 @@ import * as d3Format from 'd3-format';
// Preparatory Steps
// ----------------------------------------------------------------------

class NumCoercible {
a: number;

constructor(a: number) {
this.a = a;
}
valueOf() {
return this.a;
}
}

const numeric: NumCoercible = new NumCoercible(10);

let num: number;

let formatFn: (n: number) => string;
Expand All @@ -30,6 +43,12 @@ formatFn = d3Format.format('.0%');

formatFn = d3Format.formatPrefix(',.0', 1e-6);

d3Format.format('.0%')(10);
d3Format.format('.0%')(numeric);

d3Format.formatPrefix(',.0', 1e-6)(10);
d3Format.formatPrefix(',.0', 1e-6)(numeric);

// ----------------------------------------------------------------------
// Test Format Specifier
// ----------------------------------------------------------------------
Expand All @@ -43,7 +62,7 @@ const symbol: '$' | '#' | '' = specifier.symbol;
const zero: boolean = specifier.zero;
const width: number | undefined = specifier.width;
const comma: boolean = specifier.comma;
const precision: number = specifier.precision;
const precision: number | undefined = specifier.precision;
const type: 'e' | 'f' | 'g' | 'r' | 's' | '%' | 'p' | 'b' | 'o' | 'd' | 'x' | 'X' | 'c' | '' | 'n' = specifier.type;

const formatString: string = specifier.toString();
Expand Down
41 changes: 23 additions & 18 deletions types/d3-format/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,15 @@ export interface FormatLocaleDefinition {
*/
grouping: number[];
/**
* The currency prefix and suffix (e.g., ["$", ""])
* The currency prefix and suffix (e.g., ["$", ""]).
*/
currency: [string, string];
/**
* An optional array of ten strings to replace the numerals 0-9.
*/
numerals?: string[];
/**
* An optional symbol to replace the `percent` suffix; the percent suffix (defaults to "%")
* An optional symbol to replace the `percent` suffix; the percent suffix (defaults to "%").
*/
percent?: string;
}
Expand All @@ -44,20 +44,22 @@ export interface FormatLocaleObject {
* Returns a new format function for the given string specifier. The returned function
* takes a number as the only argument, and returns a string representing the formatted number.
*
* @param specifier A Specifier string
* @param specifier A Specifier string.
* @throws Error on invalid format specifier.
*/
format(specifier: string): (n: number) => string;
format(specifier: string): (n: number | { valueOf(): number }) => string;

/**
* Returns a new format function for the given string specifier. The returned function
* takes a number as the only argument, and returns a string representing the formatted number.
* The returned function will convert values to the units of the appropriate SI prefix for the
* specified numeric reference value before formatting in fixed point notation.
*
* @param specifier A Specifier string
* @param specifier A Specifier string.
* @param value The reference value to determine the appropriate SI prefix.
* @throws Error on invalid format specifier.
*/
formatPrefix(specifier: string, value: number): (n: number) => string;
formatPrefix(specifier: string, value: number): (n: number | { valueOf(): number }) => string;
}

/**
Expand All @@ -71,7 +73,7 @@ export interface FormatSpecifier {
*/
fill: string;
/**
* Alignment used for format, as set by choosing one of the following
* Alignment used for format, as set by choosing one of the following:
*
* '>' - Forces the field to be right-aligned within the available space. (Default behavior).
* '<' - Forces the field to be left-aligned within the available space.
Expand All @@ -83,9 +85,9 @@ export interface FormatSpecifier {
* The sign can be:
*
* '-' - nothing for positive and a minus sign for negative. (Default behavior.)
* '+' - a plus sign for positive and a minus sign for negative.
* '+' - a plus sign for positive and a minus sign for negative.
* '(' - nothing for positive and parentheses for negative.
* ' '(space) - a space for positive and a minus sign for negative.
* ' ' (space) - a space for positive and a minus sign for negative.
*
*/
sign: '-' | '+' | '(' | ' ';
Expand All @@ -94,7 +96,7 @@ export interface FormatSpecifier {
*
* '$' - apply currency symbols per the locale definition.
* '#' - for binary, octal, or hexadecimal notation, prefix by 0b, 0o, or 0x, respectively.
* ''(none) - no symbol.
* '' (none) - no symbol. (Default behavior.)
*/
symbol: '$' | '#' | '';
/**
Expand All @@ -116,9 +118,9 @@ export interface FormatSpecifier {
* it defaults to 6 for all types except '' (none), which defaults to 12.
* Precision is ignored for integer formats (types 'b', 'o', 'd', 'x', 'X' and 'c').
*
* See precisionFixed and precisionRound for help picking an appropriate precision
* See precisionFixed and precisionRound for help picking an appropriate precision.
*/
precision: number;
precision: number | undefined;
/**
* The available type values are:
*
Expand All @@ -137,7 +139,7 @@ export interface FormatSpecifier {
* 'c' - converts the integer to the corresponding unicode character before printing.
* '' (none) - like g, but trim insignificant trailing zeros.
*
* The type 'n' is also supported as shorthand for ',g'. For the 'g', 'n' and ''(none) types,
* The type 'n' is also supported as shorthand for ',g'. For the 'g', 'n' and '' (none) types,
* decimal notation is used if the resulting string would have precision or fewer digits; otherwise, exponent notation is used.
*/
type: 'e' | 'f' | 'g' | 'r' | 's' | '%' | 'p' | 'b' | 'o' | 'd' | 'x' | 'X' | 'c' | '' | 'n';
Expand Down Expand Up @@ -173,25 +175,27 @@ export function formatDefaultLocale(defaultLocale: FormatLocaleDefinition): Form
* The general form of a specifier is [[fill]align][sign][symbol][0][width][,][.precision][type].
* For reference, an explanation of the segments of the specifier string, refer to the FormatSpecifier interface properties.
*
* @param specifier A Specifier string
* @param specifier A Specifier string.
* @throws Error on invalid format specifier.
*/
export function format(specifier: string): (n: number) => string;
export function format(specifier: string): (n: number | { valueOf(): number }) => string;

/**
* Returns a new format function for the given string specifier. The returned function
* takes a number as the only argument, and returns a string representing the formatted number.
* The returned function will convert values to the units of the appropriate SI prefix for the
* specified numeric reference value before formatting in fixed point notation.
*
* Uses the current default locale.
* Uses the current default locale.
*
* The general form of a specifier is [[fill]align][sign][symbol][0][width][,][.precision][type].
* For reference, an explanation of the segments of the specifier string, refer to the FormatSpecifier interface properties.
*
* @param specifier A Specifier string
* @param specifier A Specifier string.
* @param value The reference value to determine the appropriate SI prefix.
* @throws Error on invalid format specifier.
*/
export function formatPrefix(specifier: string, value: number): (n: number) => string;
export function formatPrefix(specifier: string, value: number): (n: number | { valueOf(): number }) => string;

/**
* Parses the specified specifier, returning an object with exposed fields that correspond to the
Expand All @@ -201,6 +205,7 @@ export function formatPrefix(specifier: string, value: number): (n: number) => s
* For reference, an explanation of the segments of the specifier string, refer to the FormatSpecifier interface properties.
*
* @param specifier A specifier string.
* @throws Error on invalid format specifier.
*/
export function formatSpecifier(specifier: string): FormatSpecifier;

Expand Down
Loading