Skip to content

ts options #1024

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

Closed
wants to merge 4 commits into from
Closed

ts options #1024

wants to merge 4 commits into from

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Aug 8, 2022

Typescript for data (the expected shapes of data) and options (valueof, arrayify), with tooling and unit tests.

Was researched initially in #1008, and confirmed to work with all the transforms and more in #1005.

@Fil Fil requested review from duaneatat, mbostock and mythmon August 8, 2022 14:46
@Fil Fil mentioned this pull request Aug 8, 2022
1 task
Copy link
Member

@mythmon mythmon left a comment

Choose a reason for hiding this comment

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

These types look generally very good. I'm excited about being able to have a generic Data<T>, and tsd seems very useful.

I tried to get VSCode to give me the field-level auto complete that this should enable. I'm not sure I got it right, but this is my test file:

// setup
import {Data, Row} from "./data";
import { valueof } from "./options";

interface DatumShape extends Row {
  name: string;
  hairColor: "red" | "green";
  height: number;
  shoeSize: number;
}

const data: Data<DatumShape> = [
  {name: "Bob", hairColor: "red", height: 180, shoeSize: 11.5},
  {name: "Sarah", hairColor: "green", height: 170, shoeSize: 7},
] as const;

// test autocomplete
valueof(data, "eyeColor");
valueof(data, "")

I assume that the second parameter to valueof is similar to the channel options we'd pass in a fully TS-ified version of Plot. Based on my hopes for the end result, I'd expect eyeColor on the second to last line to be an error, and for the empty string on the last line to autocomplete to one of name, hairColor, height, or shoeSize. Neither of these seems to be working. I'm not sure if I'm using the API wrong, or if something else is missing.

I found writing the type of data to be a bit tricky. I'm not sure I got it right.

I also tried this less-typed version, which is probably closer to what we'll get in reality.

import { valueof } from "./options";

const data = [
  {name: "Bob", hairColor: "red", height: 180, shoeSize: 11.5},
  {name: "Sarah", hairColor: "green", height: 170, shoeSize: 7},
] as const;

valueof(data, "eyeColor");
valueof(data, "")

This did better. The usage of eyeColor gave me an error, though I still don't get field auto completions on the empty string. I think even this much would be a great boon to end users in the complete conversion.

Comment on lines +113 to +119
// Some channels may allow a string constant to be specified; to differentiate
// string constants (e.g., "red") from named fields (e.g., "date"), this
// function tests whether the given value is a CSS color string and returns a
// tuple [channel, constant] where one of the two is undefined, and the other is
// the given value. If you wish to reference a named field that is also a valid
// CSS color, use an accessor (d => d.red) instead.
export function maybeColorChannel<T extends Datum>(
Copy link
Member

Choose a reason for hiding this comment

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

Who is the target audience for this comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the internal comment from before

const TypedArray = Object.getPrototypeOf(Uint8Array);
const objectToString = Object.prototype.toString;

export function valueof<T extends undefined | null>(d: T, v: Accessor<T, any>, t?: ArrayType): T;
Copy link
Member

Choose a reason for hiding this comment

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

In my testing, it seems that when VSCode gives type hints for a function with multiple overrides, it shows them in the order specified in the code. Is there anything else that should affect the order of these overrides? If not, I'd suggest ordering them based on how commonly we expect them to be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some of them are more precise and must be before a less precise version that would catch the more precise case. So if we want to reorder them, we'll have to be extra careful.

@Fil
Copy link
Contributor Author

Fil commented Aug 8, 2022

Thanks for testing this Michael! Here are a few cases that seem to work for me.
Capture d’écran 2022-08-08 à 18 27 54

However I don't have autocompletion on the empty string, which I think used to work in an earlier stage 😅

@Fil
Copy link
Contributor Author

Fil commented Aug 23, 2022

A related error is that the popup here says that the value is null | undefined, when it will actually be an array of undefined

Capture d’écran 2022-08-23 à 14 36 26

@Fil
Copy link
Contributor Author

Fil commented Aug 23, 2022

@@ -16,7 +16,8 @@ export type Row = Record<string, Value>;
  */
 export type Datum = Row | Value | Value[];
 export type FieldNames<T> = T extends Row
-  ? keyof T
+  ? // eslint-disable-next-line @typescript-eslint/ban-types
+    keyof T | (string & {})
   : T extends Value[]
   ? // eslint-disable-next-line @typescript-eslint/ban-types
     "length" | "0" | "1" | "2" | (string & {})

With this patch, we get autocomplete to work again, and the issue or "wrongfully giving undefined | null" above goes away too. However the price to pay is that we lose the ts error on:
valueof([{one: "one", two: "two"}], "red");

I think this makes sense.

@Fil Fil marked this pull request as draft February 6, 2023 13:01
@Fil
Copy link
Contributor Author

Fil commented Feb 6, 2023

I hope we can restart work on this soon, with a simpler approach (not trying to pass the shape of data through all the code).

@mbostock
Copy link
Member

Probably superseded by #1320 which goes the d.ts route, but would be nice to repurpose the type tests here?

Fil added a commit that referenced this pull request Mar 14, 2023
@Fil Fil mentioned this pull request Mar 14, 2023
2 tasks
@Fil
Copy link
Contributor Author

Fil commented Mar 14, 2023

l33t tests at #1337

@Fil Fil closed this Mar 14, 2023
@Fil Fil deleted the ts-options branch March 14, 2023 15:59
mbostock added a commit that referenced this pull request Mar 16, 2023
* tests salvaged from #1024

* fix crash in valueof when the data is not iterable

* avoid the nullish test in map

* move null test back to map

* consolidate tests

* remove "valid input data" type tests

* test array value

---------

Co-authored-by: Mike Bostock <[email protected]>
frontend-provider pushed a commit to frontend-provider/plot that referenced this pull request Sep 20, 2023
* tests salvaged from observablehq/plot#1024

* fix crash in valueof when the data is not iterable

* avoid the nullish test in map

* move null test back to map

* consolidate tests

* remove "valid input data" type tests

* test array value

---------

Co-authored-by: Mike Bostock <[email protected]>
backend-devloper pushed a commit to backend-devloper/plot that referenced this pull request Nov 24, 2023
* tests salvaged from observablehq/plot#1024

* fix crash in valueof when the data is not iterable

* avoid the nullish test in map

* move null test back to map

* consolidate tests

* remove "valid input data" type tests

* test array value

---------

Co-authored-by: Mike Bostock <[email protected]>
chaichontat pushed a commit to chaichontat/plot that referenced this pull request Jan 14, 2024
* tests salvaged from observablehq#1024

* fix crash in valueof when the data is not iterable

* avoid the nullish test in map

* move null test back to map

* consolidate tests

* remove "valid input data" type tests

* test array value

---------

Co-authored-by: Mike Bostock <[email protected]>
tigrevol8888 added a commit to tigrevol8888/plot that referenced this pull request Jul 5, 2024
* tests salvaged from observablehq/plot#1024

* fix crash in valueof when the data is not iterable

* avoid the nullish test in map

* move null test back to map

* consolidate tests

* remove "valid input data" type tests

* test array value

---------

Co-authored-by: Mike Bostock <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants