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

Typescript #880

Merged
merged 14 commits into from
Jun 29, 2022
13 changes: 7 additions & 6 deletions .eslintrc.json
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
{
"extends": "eslint:recommended",
"parserOptions": {
"sourceType": "module",
"ecmaVersion": 2020
},
"extends": ["eslint:recommended", "plugin:@typescript-eslint/recommended"],
"plugins": ["@typescript-eslint"],
"parser": "@typescript-eslint/parser",
"env": {
"es6": true,
"node": true,
Expand All @@ -15,6 +13,9 @@
"no-sparse-arrays": 0,
"no-unexpected-multiline": 0,
"comma-dangle": ["error", "never"],
"semi": [2, "always"]
"semi": [2, "always"],
"@typescript-eslint/no-empty-function": 0,
"@typescript-eslint/no-this-alias": 0,
"@typescript-eslint/no-unused-vars": ["error", {"ignoreRestSiblings": true}]
}
}
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,5 @@ dist/
node_modules/
test/output/*-changed.svg
test/output/*-changed.html
tsconfig.tsbuildinfo
yarn-error.log
14 changes: 14 additions & 0 deletions .mocharc.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
{
"require": ["module-alias/register.js", "ts-node/register"],
"loader": "ts-node/esm",
"extensions": ["js", "ts"],
"spec": [
"test/**/*-test.*",
"test/plot.js"
],
"watch-files": [
"bundle.js",
"test",
"src"
]
}
28 changes: 22 additions & 6 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,14 @@
},
"license": "ISC",
"type": "module",
"main": "src/index.js",
"module": "src/index.js",
"main": "dist/index.js",
"module": "dist/index.js",
"jsdelivr": "dist/plot.umd.min.js",
"unpkg": "dist/plot.umd.min.js",
"exports": {
"mocha": "./src/index.js",
"umd": "./dist/plot.umd.min.js",
"default": "./src/index.js"
"default": "./dist/index.js"
},
"repository": {
"type": "git",
Expand All @@ -25,8 +26,11 @@
"src/**/*.js"
],
"scripts": {
"test": "mkdir -p test/output && mocha -r module-alias/register 'test/**/*-test.js' test/plot.js && eslint src test",
"prepublishOnly": "rm -rf dist && rollup -c",
"test": "yarn test:typecheck && yarn test:lint && yarn test:mocha",
"test:mocha": "mkdir -p test/output && mocha --conditions=mocha",
"test:lint": "eslint src test",
"test:typecheck": "tsc --noEmit",
"prepublishOnly": "rm -rf dist && rollup -c && tsc",
"postpublish": "git push && git push --tags",
"dev": "vite"
},
Expand All @@ -38,15 +42,27 @@
"@rollup/plugin-commonjs": "22",
"@rollup/plugin-json": "4",
"@rollup/plugin-node-resolve": "13",
"@rollup/plugin-typescript": "^8.3.2",
"@types/d3": "^7.4.0",
"@types/expect": "^24.3.0",
"@types/mocha": "^9.1.1",
"@types/node": "^17.0.35",
"@typescript-eslint/eslint-plugin": "^5.25.0",
"@typescript-eslint/parser": "^5.25.0",
"canvas": "2",
"eslint": "8",
"eslint": "^8.16.0",
"glob": "^8.0.3",
"htl": "0.3",
"js-beautify": "1",
"jsdom": "19",
"mocha": "10",
"module-alias": "2",
"rollup": "2",
"rollup-plugin-terser": "7",
"ts-node": "^10.8.0",
"tslib": "^2.4.0",
"typescript": "^4.6.4",
"typescript-module-alias": "^1.0.2",
"vite": "2"
},
"dependencies": {
Expand Down
2 changes: 2 additions & 0 deletions rollup.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import commonjs from "@rollup/plugin-commonjs";
import json from "@rollup/plugin-json";
import node from "@rollup/plugin-node-resolve";
import * as meta from "./package.json";
import typescript from "@rollup/plugin-typescript";

const filename = meta.name.split("/").pop();

Expand All @@ -26,6 +27,7 @@ const config = {
banner: `// ${meta.name} v${meta.version} Copyright ${copyrights.join(", ")}`
},
plugins: [
typescript(),
commonjs(),
json(),
node()
Expand Down
56 changes: 45 additions & 11 deletions src/curve.js → src/curve.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,41 @@ import {
curveStepAfter,
curveStepBefore
} from "d3";
import type {
CurveFactory,
CurveBundleFactory,
CurveCardinalFactory,
CurveCatmullRomFactory
} from "d3";

type CurveFunction =
| CurveFactory
| CurveBundleFactory
| CurveCardinalFactory
| CurveCatmullRomFactory;
type CurveName =
| "basis"
| "basis-closed"
| "basis-open"
| "bundle"
| "bump-x"
| "bump-y"
| "cardinal"
| "cardinal-closed"
| "cardinal-open"
| "catmull-rom"
| "catmull-rom-closed"
| "catmull-rom-open"
| "linear"
| "linear-closed"
| "monotone-x"
| "monotone-y"
| "natural"
| "step"
| "step-after"
| "step-before";

const curves = new Map([
const curves = new Map<CurveName, CurveFunction>([
["basis", curveBasis],
["basis-closed", curveBasisClosed],
["basis-open", curveBasisOpen],
Expand All @@ -44,19 +77,20 @@ const curves = new Map([
["step-before", curveStepBefore]
]);

export function Curve(curve = curveLinear, tension) {
export function Curve(
curve: CurveName | CurveFunction = curveLinear,
tension?: number
): CurveFunction {
if (typeof curve === "function") return curve; // custom curve
const c = curves.get(`${curve}`.toLowerCase());
const c = curves.get(`${curve}`.toLowerCase() as CurveName);
if (!c) throw new Error(`unknown curve: ${curve}`);
Comment on lines +85 to 86
Copy link
Contributor

Choose a reason for hiding this comment

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

It works, but the logic escapes me a little: if curve is not a curveName, the curves.get(…) must still be allowed to proceed.
For example Plot.line(data, {curve: "BASIS"}) is correct, as is Plot.line(data, {curve: {toString: () => "BASIS"}}).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 makes sense!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

spoke to Fil about this, we can have the types be case-sensitive but still let the js be more permissive, so as it's written is good

if (tension !== undefined) {
switch (c) {
case curveBundle: return c.beta(tension);
case curveCardinalClosed:
case curveCardinalOpen:
case curveCardinal: return c.tension(tension);
case curveCatmullRomClosed:
case curveCatmullRomOpen:
case curveCatmullRom: return c.alpha(tension);
if ("beta" in c) {
return c.beta(tension);
} else if ("tension" in c) {
return c.tension(tension);
} else if ("alpha" in c) {
return c.alpha(tension);
}
}
return c;
Expand Down
29 changes: 0 additions & 29 deletions src/defined.js

This file was deleted.

29 changes: 29 additions & 0 deletions src/defined.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import {ascending, descending, type Primitive} from "d3";

export function defined(x: Primitive | undefined): boolean {
return x != null && !Number.isNaN(x);
}

export function ascendingDefined(a: Primitive | undefined, b: Primitive | undefined): number {
return +defined(b) - +defined(a) || ascending(a, b);
}

export function descendingDefined(a: Primitive | undefined, b: Primitive | undefined): number {
return +defined(b) - +defined(a) || descending(a, b);
}

export function nonempty(x: unknown): boolean {
return x != null && `${x}` !== "";
}

export function finite(x: number): number {
return isFinite(x) ? x : NaN;
}

export function positive(x: number): number {
return x > 0 && isFinite(x) ? x : NaN;
}

export function negative(x: number): number {
return x < 0 && isFinite(x) ? x : NaN;
}
36 changes: 0 additions & 36 deletions src/format.js

This file was deleted.

37 changes: 37 additions & 0 deletions src/format.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/* eslint-disable @typescript-eslint/no-explicit-any */
import {format as isoFormat} from "isoformat";
import {string} from "./options.js";
import {memoize1} from "./memoize.js";

const numberFormat = memoize1<Intl.NumberFormat>((locale: string | string[] | undefined) => new Intl.NumberFormat(locale));
const monthFormat = memoize1<Intl.DateTimeFormat>((locale: string | string[] | undefined, month: "numeric" | "2-digit" | "long" | "short" | "narrow" | undefined) => new Intl.DateTimeFormat(locale, {timeZone: "UTC", month}));
const weekdayFormat = memoize1<Intl.DateTimeFormat>((locale: string | string[] | undefined, weekday: "long" | "short" | "narrow" | undefined) => new Intl.DateTimeFormat(locale, {timeZone: "UTC", weekday}));

export function formatNumber(locale = "en-US"): (value: any) => string | undefined {
const format = numberFormat(locale);
return (i: any) => i != null && !isNaN(i) ? format.format(i) : undefined;
}

export function formatMonth(locale = "en-US", month: "numeric" | "2-digit" | "long" | "short" | "narrow" | undefined = "short") {
const format = monthFormat(locale, month);
return (i: Date | number | null | undefined) => i != null && !isNaN(i = +new Date(Date.UTC(2000, +i))) ? format.format(i) : undefined;
}

export function formatWeekday(locale = "en-US", weekday: "long" | "short" | "narrow" | undefined = "short") {
const format = weekdayFormat(locale, weekday);
return (i: Date | number | null | undefined) => i != null && !isNaN(i = +new Date(Date.UTC(2001, 0, +i))) ? format.format(i) : undefined;
}

export function formatIsoDate(date: Date): string {
return isoFormat(date, "Invalid Date");
}

export function formatAuto(locale = "en-US"): (value: any) => string | number | undefined {
const number = formatNumber(locale);
return (v: any) => (v instanceof Date ? formatIsoDate : typeof v === "number" ? number : string)(v);
}

// TODO When Plot supports a top-level locale option, this should be removed
// because it lacks context to know which locale to use; formatAuto should be
// used instead whenever possible.
export const formatDefault = formatAuto();
1 change: 0 additions & 1 deletion src/math.js

This file was deleted.

1 change: 1 addition & 0 deletions src/math.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const radians: number = Math.PI / 180;
10 changes: 0 additions & 10 deletions src/memoize.js

This file was deleted.

11 changes: 11 additions & 0 deletions src/memoize.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
/* eslint-disable @typescript-eslint/no-explicit-any */
export function memoize1<T>(compute: (...rest: any[]) => T) {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of using any for the rest arguments, you can use unknown. The difference is subtle, but TS will assume anything about an any type, but it won't assume anything about unknown.

Copy link
Contributor

Choose a reason for hiding this comment

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

This sounds like very arcane knowledge, black belt! I'm still a yellow belt.

Copy link
Member

Choose a reason for hiding this comment

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

Here's a source that talks a bit about the difference: https://www.typescript-training.com/course/fundamentals-v3/11-top-and-bottom-types/

Some useful quotes:

We can see here that any is not always a “bug” or a “problem” — it just indicates maximal flexibility and the absence of type checking validation.

any is the way to tell Typescript that you don't know anything about the type, and that you want to back to the JS rules for types. It's useful, but anytime you do it you are weakening the type of the thing you're working with.

Values with an unknown type cannot be used without first applying a type guard

unknown is the way to tell Typescript that you don't know anything about the type, and that you would like it's help to prove that what you are doing is type safe. It's useful because you are asking Typescript to help make the types stronger.

let cacheValue: T, cacheKeys: any[] | undefined;
return (...keys: any[]) => {
if (cacheKeys?.length !== keys.length || cacheKeys.some((k, i) => k !== keys[i])) {
cacheKeys = keys;
cacheValue = compute(...keys);
}
return cacheValue;
};
}
8 changes: 4 additions & 4 deletions src/transforms/bin.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,10 +88,10 @@ function binn(
stroke,
x1, x2, // consumed if x is an output
y1, y2, // consumed if y is an output
domain, // eslint-disable-line no-unused-vars
cumulative, // eslint-disable-line no-unused-vars
thresholds, // eslint-disable-line no-unused-vars
interval, // eslint-disable-line no-unused-vars
domain,
cumulative,
thresholds,
interval,
...options
} = inputs;
const [GZ, setGZ] = maybeColumn(z);
Expand Down
6 changes: 6 additions & 0 deletions src/types/isoformat.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
/* eslint-disable @typescript-eslint/no-explicit-any */
declare module "isoformat" {
export function format(value: any, fallback: string): string;
export function format(value: any, fallback: any): any;
export function parse(value: string): Date;
}
2 changes: 1 addition & 1 deletion src/warnings.js → src/warnings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ export function consumeWarnings() {
return w;
}

export function warn(message) {
export function warn(message: string) {
console.warn(message);
++warnings;
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import * as Plot from "@observablehq/plot";
import assert from "assert";
import * as assert from "assert";

it("Plot.legend({color: {type:'identity'}}) returns undefined", () => {
const l = Plot.legend({color: {type: "identity"}});
Expand Down
Loading