vega / vega-lite

A concise grammar of interactive graphics, built on Vega.
https://vega.github.io/vega-lite/
BSD 3-Clause "New" or "Revised" License
4.65k stars 606 forks source link

Export all spec typings for better TypeScript support #9222

Open skokenes opened 9 months ago

skokenes commented 9 months ago

Enhancement Description

tl;dr: Vega-Lite should export all spec typings as a stable API for TS developers to use

Vega-Lite currently only exports the TopLevelSpec union type for working with specs in TypeScript. The docs here show how to use this to write a static spec.

However, if you use TS to construct or modify a spec, you will quickly hit issues with type narrowing throughout your spec as Vega-Lite relies heavily on discriminated unions. This makes it very difficult to use unless you are willing to resort to using any as a workaround for these TS errors.

See this example sandbox where trying to turn an axis off on an existing spec throws a type narrowing error.

import { TopLevelSpec } from "vega-lite";

const spec: TopLevelSpec = {
  $schema: "https://vega.github.io/schema/vega-lite/v5.json",
  width: 100,
  height: 100,
  layer: [],
};

spec.layer = [
  {
    mark: {
      type: "bar",
    },
    encoding: {
      x: {
        field: "x",
        type: "nominal",
      },
      y: {
        field: "y",
        type: "quantitative",
      },
    },
  },
];

// Type narrowing error, can't disable axis
spec.layer[0]!.encoding!.y!.axis = null;

An issue about this has been opened before: https://github.com/vega/vega-lite/issues/8900. The resolution to that issue was a recommendation to import any needed types directly from the Vega-Lite source. There are 2 issues with this:

  1. Developers may be using Vega-Lite in an environment where they only have a bundled Vega-Lite package + the exported types. With no access to src in such an environment, they can't access any of these typings.
  2. Importing types this way forces a developer to take an unstable path that developers of Vega-Lite could break at any time

Since the Vega-Lite spec itself is a stable API, the typings for that spec should also be treated as a stable API. Otherwise, working with a Vega-Lite spec in TS beyond just a single spec definition typed to TopLevelSpec is painful and hacky.

Checklist

domoritz commented 9 months ago

Thanks for filing the issue. Here is a smaller repro.

const spec: TopLevelSpec = {
  layer: []
};

// Type narrowing error, can't disable axis
spec.layer[0]!.encoding!.y!.axis = null;
  1. Importing types this way forces a developer to take an unstable path that developers of Vega-Lite could break at any time

In a way it's actually a feature. We want to be able to rename types or change how we set them up (e.g. changing generics) without having to make a major version bump of Vega-Lite. We guarantee spec compatibility but guaranteeing type compatibility would prevent us from doing improvements such as https://github.com/vega/vega-lite/pull/7438. Also, we don't have the infrastructure to even test that the types are not changing between versions (while we have pretty robust tests that specs still work) so it would be a lot of work to guarantee that types are not changing.

Having said that, I think the JS ecosystem right now often doesn't guarantee type compatibility and I have experienced many packages breaking types in minor versions probably because it's difficult.

I'd be okay with exporting the types from the top level package so they can be imported but we should warn developers that the types may change in minor versions as well. Alternatively, we could set up a new package called Vega-lite-typings that exports all the types and then that package could be versioned separately.

domoritz commented 8 months ago

See also https://github.com/vega/vega-lite/issues/7069#issuecomment-973168541.

ellis commented 7 months ago

We're facing the same issue. We compose parts of specs together from different places to assemble the full spec. To achieve this, we used imports like this:

import { TimeUnit } from "vega-lite/build/src/timeunit";

However, we've now switched to ES Modules, and imports like that are no longer possible. Either of the approaches that @domoritz suggested would work for us. The main need is to have type-safety now; if the type names break later, that can be annoying, but definitely not a deal breaker.

domoritz commented 7 months ago

What about instead of exporting the types of we export a few type guards? That would make it easier to selectively export only some things we are more confident about. But it would mean we need to manually select and write the type guards.

As a stop gap, I think we can expect the types from the top level with a warning that we don't guarantee compatibility across versions.

ellis commented 7 months ago

I doubt that type guards would work well for the use case described here of building up the specs piecemeal. Perhaps you could export the types under a path like vega-lite/types-unstable to make it obvious that change is to be expected?

domoritz commented 7 months ago

I see. Are you suggesting using export maps to export types there or just a subfolder? Would another export work for your use case @skokenes?

ellis commented 5 months ago

I made the following patch to get it to work for my purposes. It's not a good general solution, though.

diff --git a/build/src/types-unstable.d.ts b/build/src/types-unstable.d.ts
new file mode 100644
index 0000000000000000000000000000000000000000..0b2b5cd12e4ac67d5ca3d66ab4566cd3e63dd533
--- /dev/null
+++ b/build/src/types-unstable.d.ts
@@ -0,0 +1,54 @@
+export type * from "./aggregate";
+export type * from "./axis";
+export type * from "./bin";
+export type * from "./channel";
+export type * from "./channeldef";
+export type * from "./compositemark/base";
+export type * from "./compositemark/boxplot";
+export type * from "./compositemark/common";
+export type * from "./compositemark/errorband";
+export type * from "./compositemark/errorbar";
+export type * from "./compositemark/index";
+export type * from "./data";
+export type * from "./datetime";
+export type * from "./encoding";
+export type * from "./expr";
+export type * from "./guide";
+export type * from "./header";
+export type * from "./impute";
+export type * from "./index";
+export type * from "./legend";
+export type * from "./log/index";
+export type * from "./log/message";
+export type * from "./logical";
+// export type * from "./mark";
+export type * from "./normalize/base";
+export type * from "./normalize/core";
+export type * from "./normalize/index";
+export type * from "./normalize/pathoverlay";
+export type * from "./normalize/repeater";
+export type * from "./normalize/ruleforrangedline";
+export type * from "./normalize/selectioncompat";
+export type * from "./normalize/toplevelselection";
+export type * from "./parameter";
+export type * from "./predicate";
+export type * from "./projection";
+export type * from "./resolve";
+export type * from "./scale";
+export type * from "./selection";
+export type * from "./sort";
+export type * from "./spec/base";
+export type * from "./spec/concat";
+export type * from "./spec/facet";
+export type * from "./spec/index";
+export type * from "./spec/layer";
+export type * from "./spec/map";
+export type * from "./spec/repeat";
+export type * from "./spec/toplevel";
+export type * from "./spec/unit";
+export type * from "./stack";
+export type * from "./timeunit";
+export type * from "./title";
+// export type * from "./transform";
+// export type * from "./type";
+export type * from "./util";
diff --git a/package.json b/package.json
index 568408feafe13cde070f1bdf127af8f622fb0676..a69242062beb911c5d4391b15643f9ea429fdff6 100644
--- a/package.json
+++ b/package.json
@@ -20,6 +20,20 @@
   "jsdelivr": "build/vega-lite.min.js",
   "module": "build/src/index",
   "types": "build/src/index.d.ts",
+  "type": "module",
+  "exports": {
+    ".": {
+      "import": {
+        "types": "./build/src/index.d.ts",
+        "default": "./build/src/index.js"
+      },
+      "require": {
+        "types": "./build/src/index.d.ts",
+        "default": "./build/vega-lite.js"
+      }
+    },
+    "./types-unstable": "./build/src/types-unstable.d.ts"
+  },
   "bin": {
     "vl2pdf": "./bin/vl2pdf",
     "vl2png": "./bin/vl2png",