gajus / table

Formats data into a string table.
Other
904 stars 76 forks source link

Indexable has poor typing #156

Closed yankeeinlondon closed 3 years ago

yankeeinlondon commented 3 years ago

I wasn't sure how to phrase this error because in the past I've been very surprised to see that when defining a table's configuration you were asked to put in a object/dictionary but with numeric keys. Since upgrading to the most recently typed version of Table I wrote the following code:

const config: TableUserConfig = {
        columns: [
          { width: 30, alignment: "left" },
          { width: 5, alignment: "center" },
          { width: 60, alignment: "left" },
        ],
      };

      console.log(table([["Name", "Version", "Description"], ...data], config));

There were no type error but then I got a runtime error about an invalid configuration. I investigated and sure enough the typing depends on being Indexable and here's a code snippet of two test data structures sitting next to your Indexable:

export declare type Indexable<T> = {
    readonly [index: number]: T;
};

const t1: Indexable<{test: string}> = [{test: "foo"}, {test: "bar"}];
const t2: Indexable<{test: string}> = { 1: {test: "foo"}, 2:{test: "bar"}};

Neither has a typing error and therefore one of two things must be true:

  1. The type is unnecessarily permissive and you should be required to use object syntax but use numeric keys
  2. The run time checker is objecting to a type which should be fine

I must say the array syntax feel MUCH more natural than what can actually be used today at run time but either way one of the two really must change.

nam-hle commented 3 years ago

While migrating to TypeScript, I want to keep all behaviors are the same so if the old JS code throws a runtime error then the TS one should throw too. But you're right, the table should accept the array columns (#134) but the number index object is also useful in some cases when the user just wants to define some column configuration.

yankeeinlondon commented 3 years ago

Well my code above is trying to do what you mention (aka, trying to configure column config) ... and asking people to choose a JS object but use numeric keys is an anti-pattern in JS. What is an array but an object (arrays are objects in JS) with numeric keys? I understand your point about focus and doing one thing at a time but you're in a situation currently where you must do one thing ... but changing the typing would then force you to make a breaking change later. Why not just let the run time be more permissive?

If you decide that you want to stick to your guns and just focus on the Typescript then please change the typing to block the use of an array ... if this can even be done. I presume the following would work:

export type Indexable<T, O extends object> = {
  readonly  [index: number]: T;
}  & O;

note: above is untested

Ultimately what you're doing may have been accepted by your users but I still remember being surprised about it in the past and it took me a long time to figure out what you actually wanted. Good typing should guarentee that you can't make mistakes like this. This type in particular because the API cuts against the grain in terms of conventional use of dictionaries versus arrays.

yankeeinlondon commented 3 years ago

As soon as I pressed enter I realized ... you probably can NOT type what you're doing and my example will not work because an Array does extend object. This should tell you something though ;)

nam-hle commented 3 years ago

I don't think a workaround that ensures the user provides the "number object" type is a good solution. As mentioned, the array should be supported ASAP so everyone will be happy. Cheers. 🥰🥰