Add type definitions

[TaylorBeeston]

Closes GH-20.

This PR adds the types directory that contains an index.d.ts file that allows TypeScript projects to use this package more easily. Additionally, there are some tests to ensure the correctness of this type definition, and some boilerplate associated with dtslint. package.json has been updated to include dtslint as a dev dependency. There is also the addition of the test-types script to run dtslint, and index.d.ts is exported as the types file for this project.

One thing I would like to note is that if you currently tried to use lib/github.json as is written in the Readme, TypeScript will make your life difficult due to the input attribute. The reason for this is because I tried to type the attributes object as strongly as I could by using the type {[key: string]: Array<string | [string, ...any[]]>}. This is canonically correct, allowing shapes such as

{
  input: [
    ["type", "checkbox"],
    ["disabled", true],
  ],
  div: [
    ["className", "class1", "class2", "class3"]
  ]
}

Which is how I interpreted

Instead of a single string (such as type), which allows any property value of that property name, it’s also possible to provide an array (such as ['type', 'checkbox']), where the first entry is the property name, and the other entries are allowed property values.

from the Readme.

However, TS will infer the type Array<Array<string | boolean>> for attributes from the JSON object and decide that the two types are incompatible. To defeat this in the tests, I simply inlined the Github JSON and explicitly defined it as the correct type. If it was needed to be used, you'd have to type out

sanitize(tree, (schema as unknown) as sanitize.Schema)

Which is pretty ugly in my opinion.

A solution to this could be to loosen the attributes type definition, but that comes with its own set of problems.

[ChristianMurphy]

Thanks for getting this started @TaylorBeeston!

[TaylorBeeston]

@ChristianMurphy good catch! I was just following the types listed in the Readme. I went ahead and added an HTMLValues type

[ChristianMurphy]

LGTM, thanks @TaylorBeeston! 🙇

[BarryThePenguin]

🦠 👍

[TaylorBeeston]

I went ahead and copied over the Readme descriptions as doc comments! Should be good to go!

[ChristianMurphy]

LGTM, thanks @TaylorBeeston! 🙇

[wooorm]

Sweet, thanks @TaylorBeeston. Landed in 3.0.0!

[leafac]

Thanks @TaylorBeeston for putting this together. I started using these types and I think I’d rather have looser types for attributes. I was bit by this and I think it’s one of those cases in which convenience outweighs stricter type checking. You opt out of stricter type checking with all those type casts anyway.

I know I’m half a year late to the party, but can I interest you in a pull request to make this happen?

This is what I had to end up writing:

import rehypeSanitize from "rehype-sanitize";
import hastUtilSanitize from "hast-util-sanitize";
import hastUtilSanitizeGitHubSchema from "hast-util-sanitize/lib/github.json";
import deepMerge from "deepmerge";

// ...

.use(
  rehypeSanitize,
  (deepMerge(hastUtilSanitizeGitHubSchema, {
    attributes: {
      code: ["className"],
      span: [["className", "math-inline"]],
      div: [["className", "math-display"]],
    },
  }) as unknown) as hastUtilSanitize.Schema
)

I wish to get rid of the as unknown as hastUtilSanitize.Schema part.

[ChristianMurphy]

I don't think the types should be loosened, typescript should be strongly typed by default, and people can opt out with @ts-ignore or casting if looser types are wanted.
To your particular example, you don't need to do casting, deepmerge is generic, and can do a type safe assertion on the return value.
https://codesandbox.io/s/elated-kare-o7xdk

If you're interested in ensuring hast-util-sanitize/lib/github.json has a valid type, a PR adding a typing for the file would be welcome.

[leafac]

Thanks for the detailed explanation! I’m happy that we don’t need the as unknown as hastUtilSanitize.Schema.

I tried @ts-ignore like you showed in CodeSandbox, but it didn’t work. TypeScript was still giving me giant error messages.

The following worked:

.use(
  rehypeSanitize,
  deepMerge<hastUtilSanitize.Schema>(hastUtilSanitizeGitHubSchema as any, {
    attributes: {
      code: ["className"],
      span: [["className", "math-inline"]],
      div: [["className", "math-display"]],
    },
  })
)

It’s already better than what I had before.

If I understand this correctly the next step to get rid of the as any is to provide types for hast-util-sanitize/lib/github.json. I don’t know how I’d declare the types of a JSON file, but I’ll add this to my TODO list, come back here one day, learn how to do this, and do it. (Help is appreciated 😃)

[ChristianMurphy]

Some packages, like https://github.com/sindresorhus/spdx-license-list will mirror json files with js files that can be more easily typed.
An alternative would be to check upstream in typescript itself to see the status/progress on resolveJsonModule to see if it could support json files provided by npm packages as well as files directly in a project.

[leafac]

Thank you for your help 😃

Some packages, like https://github.com/sindresorhus/spdx-license-list will mirror json files with js files that can be more easily typed.

Thanks, I’ll check that out…

An alternative would be to check upstream in typescript itself to see the status/progress on resolveJsonModule to see if it could support json files provided by npm packages as well as files directly in a project.

I’m not sure I understand what you mean. With "resolveJsonModule": true enabled in tsconfig.json I can write import hastUtilSanitizeGitHubSchema from "hast-util-sanitize/lib/github.json";. So, as I understand it, TypeScript supports JSON files provided by npm packages. The problem is the type of hastUtilSanitizeGitHubSchema, which seems to be too specific (inferred from the shape of the JSON itself), for example:

"attributes": {
    a: string[];
    img: string[];
    input: (string | boolean)[][];
    li: string[][];
    div: string[];
    blockquote: string[];
    del: string[];
    ins: string[];
    q: string[];
    "*": string[];
}

I think the type should be something like "attributes": (string | string[])[].

Does that make sense?

[ChristianMurphy]

I’m not sure I understand what you mean. With "resolveJsonModule": true enabled in tsconfig.json

That could be, a codesandbox would be helpful
the example I shared before https://codesandbox.io/s/elated-kare-o7xdk?file=/tsconfig.json has "resolveJsonModule": true enabled, but does not resolve. 🤷‍♂️

The problem is the type of hastUtilSanitizeGitHubSchema, which seems to be too specific

Too specific, as in incompatible?
Perhaps, I think the piece you believe is incompatible is

input: (string | boolean)[][];

?

If so there is a test case in the type tests, which I believe, covers this case, which is passing:

hast-util-sanitize/types/hast-util-sanitize-tests.ts

Lines 194 to 197 in 4dd66cc

	input: {
	type: 'checkbox',
	disabled: true
	}

Again, a code sandbox would be helpful for tracing the issue.

[leafac]

Here’s a repository showing what I mean: https://github.com/leafac/hast-util-sanitize--21

I’d like to provide you with a CodeSandbox, but it appears that CodeSandbox may be misbehaving. I mean, there’s an issue in the path in the CodeSandbox you provided (hast-util-sanitize/lib/github.json.json, note the .json.json), but even after I fixed that and tried a bunch of other things it didn’t work.

In any case, here’s the error I’m referring to when you use import instead of require on the JSON:

src/index.ts:17:42 - error TS2345: Argument of type '{ strip: string[]; clobberPrefix: string; clobber: string[]; ancestors: { tbody: string[]; tfoot: string[]; thead: string[]; td: string[]; th: string[]; tr: string[]; }; protocols: { href: string[]; cite: string[]; src: string[]; longDesc: string[]; }; tagNames: string[]; attributes: { ...; }; required: { ...; }; }' is not assignable to parameter of type 'Partial<Schema>'.
  Types of property 'attributes' are incompatible.
    Type '{ a: string[]; img: string[]; input: (string | boolean)[][]; li: string[][]; div: string[]; blockquote: string[]; del: string[]; ins: string[]; q: string[]; "*": string[]; }' is not assignable to type 'Record<string, (string | [string, ...(string | number | boolean)[]])[]>'.
      Property '"input"' is incompatible with index signature.
        Type '(string | boolean)[][]' is not assignable to type '(string | [string, ...(string | number | boolean)[]])[]'.
          Type '(string | boolean)[]' is not assignable to type 'string | [string, ...(string | number | boolean)[]]'.
            Type '(string | boolean)[]' is not assignable to type '[string, ...(string | number | boolean)[]]'.
              Property '0' is optional in type '(string | boolean)[]' but required in type '[string, ...(string | number | boolean)[]]'.

17       deepMerge<hastUtilSanitize.Schema>(hastUtilSanitizeGitHubSchema, {
                                            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This error message is a bit nightmarish, but I believe it’s saying that we’re both correct: both the attributes and input are causing issues…

[ChristianMurphy]

Right, and I see that as a limitation of TypeScript, one which is documented and being discussed in microsoft/TypeScript#32063.
I think @TaylorBeeston's typing is correct, and that the JSON type should be narrowed, rather than the library typing being broadened.
#21 (comment) could be a work around.

[leafac]

Right, and I see that as a limitation of TypeScript, one which is documented and being discussed in microsoft/TypeScript#32063.

👍 Got it. Thanks for the information.

I think @TaylorBeeston's typing is correct, and that the JSON type should be narrowed, rather than the library typing being broadened.

I agree.

#21 (comment) could be a work around.

I’ll look into that…

[leafac]

Okay, so I looked more into the issue and I think that the best course of action is to not bother with types for the JSON, because the whole notion of loading JSON with import seems to be a bad idea in the first place. At the very least, it requires the resolveJsonModule option to be enabled in tsconfig.json. But then if you try to emit ESM from your TypeScript (and I think that’ll be the future), you’ll run into trouble, because Node.js doesn’t support loading JSON that way (I mean, it does, but it’s behind a flag).

My conclusion: All these hoops you have to jump through to get import "<something>.json" to work seem to indicate that you’re better off loading JSON with JSON.parse(await fs.readFile(...)), which has type any anyway.

[wooorm]

We can’t do that because fs is not available in browsers: but you can!
This is a thing that the ecosystem/community will soon go through anyway though in April, when all active Node release lines support ESM. I know the Node/TC39/web folks are working on an import proposal for JSON, which might land before that