Skip to content

Type declarations #998

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 8 commits into from
Closed

Type declarations #998

wants to merge 8 commits into from

Conversation

kentr
Copy link
Contributor

@kentr kentr commented Jul 17, 2022

Here's my experimentation with declarations in case it's useful.

IMO this work is too rough to merge, but I defer to the maintainers. It needs thorough review by people who are more familiar with the Plot API and TypeScript than I am. It may conflict with the TypeScript work that's already been done.

I created the base files with tsc and then built out a handful of them. Most are still in that initial raw output state.

My goal was to create generics that would derive the allowed mark options based on the mark type, as well as utility types for readability, code reuse, etc.

It needs more tests. I stopped writing tests as the complexity increased, until it can be reviewed for correctness.

I started to incorporate the d3 scales types from DefinitelyTyped, but I saw possible errors with them. Reported here.

Related to #401.

Ping @duaneatat.

@kentr
Copy link
Contributor Author

kentr commented Jul 17, 2022

This work also needs far more documentation, IMO. My intention was to use docblocks that can be parsed for intellisense in editors and by documentation generators.

@duaneatat
Copy link
Contributor

Hi Kent,

Thanks for the contribution! Your PR is a really useful reference (specifically for the files that you paid special attention to) and thank you for reporting bugs to @types/d3! We're working on a roadmap for moving Plot to TypeScript, and want to share it with you so you can see how it's going to happen and where you could contribute.

Our plan is as follows:

  1. Start from the src/**/*.js files that have no (internal) dependencies, converting each of those "leaf" .js files to .ts traversing up the dependency tree (ending in the root file src/index.js), adding all the necessary types (e.g. no "any").
  2. Meanwhile, we'll document the public API (marks, transforms etc) with JSDocs strings that will reflect what is already documented in the README file.
  3. We hope that we'll be able to reconstruct the README file from the JSDocs strings, in order to make sure that we have only one "source of truth" (and place to do changes and edits) for the documentation.

We'll welcome small-scale suggestions (rather than the bulk output of tsc), and of course any review you want to give to our own PRs.

@Fil
Copy link
Contributor

Fil commented Jul 18, 2022

closing this, but I second what Duane says :)

@Fil Fil closed this Jul 18, 2022
@kentr
Copy link
Contributor Author

kentr commented Jul 18, 2022

@Fil @duaneatat

The roadmap sounds like a solid approach. Having the documentation in the code as a single source of truth will probably reduce the overall maintenance burden. Perhaps ObservableHQ.com will incorporate intellisense & TypeScript in the javascript editor down the road :-). As an ObservableHQ.com user myself, I think it would be as useful there as it is in the IDE.

Auto-generating the README is a worthy goal. Modularizing the docs across multiple files instead of a monolithic README might make it more digestible or open the door for a streamlined mechanism of auto-updating documentation snippets in example / tutorial notebooks on ObservableHQ.

This work I've done on type declarations is as much for my own purposes, so I started with the files related to the Plot features that I was using.

I'll pull in main to the branch and remove the unmodified tsc bulk output files, and I may do further work. Feel free to reopen this PR and change it to a different branch in the Plot repo if you find the work useful.

@mbostock
Copy link
Member

TypeScript declarations added in #1320.

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.

4 participants