fix typescript errors if typescript compiler option "exactOptionalPropertyTypes" is enabled

[montemishkin]

Huh? What is this PR??

If a developer using plot with typescript has exactOptionalPropertyTypes turned on in their own project's tsconfig, then they will get 8 typescript errors from within plot's .d.ts files. This PR adjusts plot's .d.ts files to eliminate those errors.

Other things to note

There should be no change to users who do not have that compiler option enabled. The plot repo itself does not have that compiler option enabled, and so also should not be affected, except by the unsightly / seemingly redundant | undefined I have added :/ It's a bit ugly, but given that this compiler option is recommended by typescript, seems worth supporting.

Let me know if y'all have any questions / concerns. Thanks for the useful library!

The Errors

For reference, these are the errors that are eliminated by this PR:

src/marks/axis.d.ts:55:18 - error TS2430: Interface 'AxisOptions' incorrectly extends interface 'MarkOptions'.
  Types of property 'opacity' are incompatible.
    Type 'ChannelValueSpec | undefined' is not assignable to type 'ChannelValueSpec'.
      Type 'undefined' is not assignable to type 'ChannelValueSpec'.

55 export interface AxisOptions extends GridOptions, MarkOptions, TextOptions, AxisScaleOptions {
                    ~~~~~~~~~~~

src/marks/axis.d.ts:55:18 - error TS2430: Interface 'AxisOptions' incorrectly extends interface 'TextOptions'.
  Types of property 'opacity' are incompatible.
    Type 'ChannelValueSpec | undefined' is not assignable to type 'ChannelValueSpec'.

55 export interface AxisOptions extends GridOptions, MarkOptions, TextOptions, AxisScaleOptions {
                    ~~~~~~~~~~~

src/marks/axis.d.ts:65:18 - error TS2430: Interface 'AxisXOptions' incorrectly extends interface 'TickXOptions'.
  Types of property 'opacity' are incompatible.
    Type 'ChannelValueSpec | undefined' is not assignable to type 'ChannelValueSpec'.

65 export interface AxisXOptions extends AxisOptions, TickXOptions {}
                    ~~~~~~~~~~~~

src/marks/axis.d.ts:68:18 - error TS2430: Interface 'AxisYOptions' incorrectly extends interface 'TickYOptions'.
  Types of property 'opacity' are incompatible.
    Type 'ChannelValueSpec | undefined' is not assignable to type 'ChannelValueSpec'.

68 export interface AxisYOptions extends AxisOptions, TickYOptions {}
                    ~~~~~~~~~~~~

src/marks/axis.d.ts:71:18 - error TS2430: Interface 'GridXOptions' incorrectly extends interface 'Omit<RuleXOptions, "interval">'.
  Types of property 'opacity' are incompatible.
    Type 'ChannelValueSpec | undefined' is not assignable to type 'ChannelValueSpec'.

71 export interface GridXOptions extends GridOptions, Omit<RuleXOptions, "interval"> {}
                    ~~~~~~~~~~~~

src/marks/axis.d.ts:74:18 - error TS2430: Interface 'GridYOptions' incorrectly extends interface 'Omit<RuleYOptions, "interval">'.
  Types of property 'opacity' are incompatible.
    Type 'ChannelValueSpec | undefined' is not assignable to type 'ChannelValueSpec'.

74 export interface GridYOptions extends GridOptions, Omit<RuleYOptions, "interval"> {}
                    ~~~~~~~~~~~~

src/marks/crosshair.d.ts:5:18 - error TS2430: Interface 'CrosshairOptions' incorrectly extends interface 'MarkOptions'.
  Types of property 'opacity' are incompatible.
    Type 'ChannelValueSpec | undefined' is not assignable to type 'ChannelValueSpec'.

5 export interface CrosshairOptions extends MarkOptions {
                   ~~~~~~~~~~~~~~~~

src/marks/link.d.ts:7:18 - error TS2430: Interface 'LinkOptions' incorrectly extends interface 'CurveAutoOptions'.
  Types of property 'curve' are incompatible.
    Type '"auto" | Curve | undefined' is not assignable to type '"auto" | Curve'.
      Type 'undefined' is not assignable to type '"auto" | Curve'.

7 export interface LinkOptions extends MarkOptions, MarkerOptions, CurveAutoOptions {
                   ~~~~~~~~~~~

[Fil]

If I activate this option in the repo and run yarn test:tsc, I get Found 66 errors in 35 files.

diff --git a/tsconfig.json b/tsconfig.json
index 668c98ef..7978fb37 100644
--- a/tsconfig.json
+++ b/tsconfig.json
@@ -5,6 +5,8 @@
     "noImplicitAny": false,
     "module": "Node16",
     "baseUrl": "./src",
+    "exactOptionalPropertyTypes": true,
+    "strictNullChecks": true,
     "paths": {
       "@observablehq/plot": ["index"]
     }

An alternative to adding | undefined seems to be to declare the grouped types individually, rather than reading them from an optional field from another interface. For example one could do:

export type CurveAuto = Curve | "auto";

   ...
   curve?: CurveAuto;

instead of

   curve?: CurveAutoOptions["curve"];

(My point being, if we do change something here, we should not just make the errors go away, but rather fully embrace the stricter setting with stricter code?)

[montemishkin]

@Fil - Thanks for the quick turn around :)

The number of errors

58 of those 66 errors you mention are from the strictNullChecks, whereas the remaining 8 are from the exactOptionalPropertyTypes, which go away with the | undefined addition.

For whatever reason, even though my repo using plot has strictNullChecks enabled, I don't see the remaining 58 errors, where as I do see the 8 from exactOptionalPropertyTypes.

If we want to address the strictNullChecks errors, I'd suggest they happen in a different PR.

(For future reader context: if you just turn on exactOptionalPropertyTypes, then typescript tells you you must also enable strictNullChecks)

Add | undefined vs "just copy-paste"

I don't have a strong opinion here, but these are the pros / cons of the two approaches that I currently see:

• Option 1 (adding | undefined):
  • 👍 Relatively contained in its codebase impact
  • 👎 A lil bit ugly
• Option 2 (copy-pasting):
  • 👍 Not ugly
  • 👍 More explicit
  • 👎 Decouples type definitions, such that a contributor changing one interface might not immediately realize that property should probably stay in sync with various other interfaces throughout the codebase. Not super likely, but could lead to accidental api drift between the various options object accepted by different functions.

I'd stick with Option 1 personally, but there's a fair argument for either, and I don't have a strong opinion here. Got any consequences I'm not seeing / a stronger opinion than mine?

[Fil]

My suggestion (option 3 if you will) is to export a specific type (e.g. CurveAuto) and to use it in all the places :-)

I don't see us doing only half the job. If we do support exactOptionalPropertyTypes we probably want to activate it in the repo (so that it's helpful for us and we don't forget about it); this in turn requires strictNullChecks too. It's a bit more work though.