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

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

montemishkin
Copy link

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
Copy link
Contributor

Fil commented Jun 3, 2024

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
Copy link
Author

@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
Copy link
Contributor

Fil commented Jun 4, 2024

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.

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.

2 participants