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
11 changes: 5 additions & 6 deletions .eslintrc.json
@@ -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,7 @@
"no-sparse-arrays": 0,
"no-unexpected-multiline": 0,
"comma-dangle": ["error", "never"],
"semi": [2, "always"]
"semi": [2, "always"],
"@typescript-eslint/no-empty-function": 0
}
}
2 changes: 2 additions & 0 deletions .gitignore
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
@@ -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
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 --files",
Copy link
Member

Choose a reason for hiding this comment

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

What does --conditions=mocha and --files do? I can’t find any documentation about these arguments.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, I was able to answer this myself. It’s a Node flag, not a Mocha flag, document here:

https://nodejs.org/api/packages.html#resolving-user-conditions

And it means that Node will respect the mocha export in package.json here:

"mocha": "./src/index.js",

Copy link
Member

Choose a reason for hiding this comment

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

And as for --files it doesn’t seem to have any effect so should probably be removed?

"test:lint": "eslint src test",
"test:typecheck": "yarn 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
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
48 changes: 37 additions & 11 deletions src/curve.js → src/curve.ts
Expand Up @@ -20,8 +20,32 @@ import {
curveStepAfter,
curveStepBefore
} from "d3";
import type { CurveFactory, CurveBundleFactory, CurveCardinalFactory, CurveCatmullRomFactory } from "d3";

const curves = new Map([
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<CurveName, CurveFunction>([
["basis", curveBasis],
["basis-closed", curveBasisClosed],
["basis-open", curveBasisOpen],
Expand All @@ -44,19 +68,21 @@ 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
@@ -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.

38 changes: 38 additions & 0 deletions src/format.ts
@@ -0,0 +1,38 @@
/* 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();
4 changes: 2 additions & 2 deletions src/marks/delaunay.js
Expand Up @@ -63,7 +63,7 @@ class DelaunayLink extends Mark {
const [cx, cy] = applyFrameAnchor(this, dimensions);
const xi = X ? i => X[i] : constant(cx);
const yi = Y ? i => Y[i] : constant(cy);
const mark = this;
const mark = this; // eslint-disable-line @typescript-eslint/no-this-alias

function links(index) {
let i = -1;
Expand Down Expand Up @@ -142,7 +142,7 @@ class AbstractDelaunayMark extends Mark {
const [cx, cy] = applyFrameAnchor(this, dimensions);
const xi = X ? i => X[i] : constant(cx);
const yi = Y ? i => Y[i] : constant(cy);
const mark = this;
const mark = this; // eslint-disable-line @typescript-eslint/no-this-alias

function mesh(index) {
const delaunay = Delaunay.from(index, xi, yi);
Expand Down
1 change: 0 additions & 1 deletion src/math.js

This file was deleted.

1 change: 1 addition & 0 deletions src/math.ts
@@ -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
@@ -0,0 +1,11 @@
/* eslint-disable @typescript-eslint/no-explicit-any */
export function memoize1<T>(compute: (...rest: any[]) => T) {
Copy link

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

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
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, // eslint-disable-line @typescript-eslint/no-unused-vars
cumulative, // eslint-disable-line @typescript-eslint/no-unused-vars
thresholds, // eslint-disable-line @typescript-eslint/no-unused-vars
interval, // eslint-disable-line @typescript-eslint/no-unused-vars
...options
} = inputs;
const [GZ, setGZ] = maybeColumn(z);
Expand Down
6 changes: 6 additions & 0 deletions src/types/isoformat.d.ts
@@ -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
Expand Up @@ -6,7 +6,7 @@ export function consumeWarnings() {
return w;
}

export function warn(message) {
export function warn(message: string) {
console.warn(message);
++warnings;
}