feat(unist): add generic types for better type checking by JounQin · Pull Request #54413 · DefinitelyTyped/DefinitelyTyped

JounQin · 2021-07-09T17:22:23Z

Please fill in this template.

Use a meaningful title for the pull request. Include the name of the package modified.
Test the change in your own code. (Compile and run.)
Add or edit tests to reflect the change.
Follow the advice from the readme.
Avoid common mistakes.
Run npm test <package to test>.

Select one of these and delete the others:

If adding a new definition:

The package does not already provide its own types, or cannot have its .d.ts files generated via --declaration
If this is for an npm package, match the name. If not, do not conflict with the name of an npm package.
Create it with dts-gen --dt, not by basing it on an existing project.
Represents shape of module/library correctly
tslint.json should contain { "extends": "dtslint/dt.json" }, and no additional rules.
tsconfig.json should have noImplicitAny, noImplicitThis, strictNullChecks, and strictFunctionTypes set to true.

If changing an existing definition:

Provide a URL to documentation or source code which provides context for the suggested changes: <>
If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the header.

If removing a declaration:

If a package was never on Definitely Typed, you don't need to do anything. (If you wrote a package and provided types, you don't need to register it with us.)
Delete the package's directory.
Add it to notNeededPackages.json.

typescript-bot · 2021-07-09T17:22:58Z

@JounQin Thank you for submitting this PR!

This is a live comment which I will keep updated.

1 package in this PR

unist — on npm, on unpkg
- owner-approval: @GuiltyDolphin
- 1 added owner: ✎@JounQin

@JounQin: I see that you have added yourself as an owner, are you sure you want to become an owner?

Code Reviews

Because you edited one package and updated the tests (👏), I can help you merge this PR once someone else signs off on it.

You can test the changes of this PR in the Playground.

Status

✅ No merge conflicts
✅ Continuous integration tests have passed
✅ Most recent commit is approved by type definition owners or DT maintainers

All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes.

Diagnostic Information: What the bot saw about this PR

{
  "type": "info",
  "now": "-",
  "pr_number": 54413,
  "author": "JounQin",
  "headCommitOid": "b6a5006fa5f11643892420bc198b94ebc0458716",
  "lastPushDate": "2021-07-10T12:18:02.000Z",
  "lastActivityDate": "2021-07-15T00:19:15.000Z",
  "maintainerBlessed": "Waiting for Code Reviews",
  "mergeOfferDate": "2021-07-13T01:52:30.000Z",
  "mergeRequestDate": "2021-07-15T00:19:15.000Z",
  "mergeRequestUser": "JounQin",
  "hasMergeConflict": false,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "popularityLevel": "Critical",
  "pkgInfo": [
    {
      "name": "unist",
      "kind": "edit",
      "files": [
        {
          "path": "types/unist/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/unist/unist-tests.ts",
          "kind": "test"
        }
      ],
      "owners": [
        "bizen241",
        "lujun2",
        "hrajchert",
        "wooorm",
        "rokt33r",
        "GuiltyDolphin"
      ],
      "addedOwners": [
        "JounQin"
      ],
      "deletedOwners": [],
      "popularityLevel": "Critical"
    }
  ],
  "reviews": [
    {
      "type": "approved",
      "reviewer": "GuiltyDolphin",
      "date": "2021-07-10T12:46:21.000Z",
      "isMaintainer": false
    },
    {
      "type": "stale",
      "reviewer": "remcohaszing",
      "date": "2021-07-10T11:01:56.000Z",
      "abbrOid": "afe2c0e"
    },
    {
      "type": "stale",
      "reviewer": "ChristianMurphy",
      "date": "2021-07-10T05:58:03.000Z",
      "abbrOid": "afe2c0e"
    },
    {
      "type": "stale",
      "reviewer": "wooorm",
      "date": "2021-07-09T19:40:50.000Z",
      "abbrOid": "138b76f"
    }
  ],
  "mainBotCommentID": 877339624,
  "ciResult": "pass"
}

typescript-bot · 2021-07-09T17:23:00Z

🔔 @bizen241 @lujun2 @hrajchert @wooorm @Rokt33r @GuiltyDolphin — please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

types/unist/index.d.ts

typescript-bot · 2021-07-09T17:27:20Z

@JounQin The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

Note: builds which are failing do not end up on the list of PRs for the DT maintainers to review.

GuiltyDolphin · 2021-07-09T17:30:49Z

@JounQin what do you mean by "for better type checking"?

As far as I can tell, the type-checking capabilities remain the same here - for both Parent and Literal you can just make child interfaces.

That said, I've used a similar solution in my own programming (though not with Parent, because this has lead to cyclic defaults for me), so I'm happy with this if the other owners are.

JounQin · 2021-07-09T17:43:17Z

what do you mean by "for better type checking"?

const assertStringLiertal = (node: Node): node is Liertal<string> => 'value' in node && typeof (node as Literal).value === 'string'

// before
const assertStringLiertal = (node: Node): node is Liertal & { value: string } => 'value' in node && typeof (node as Literal).value === 'string'

for both Parent and Literal you can just make child interfaces.

declare const parent: Parent

const literals = parent.children as Literal[] // error now

// after
declare const parent: Parent<Literal>

const literals = parent.children // already `Literal[]`, no error

typescript-bot · 2021-07-09T18:10:46Z

@JounQin The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

Note: builds which are failing do not end up on the list of PRs for the DT maintainers to review.

types/unist/index.d.ts

types/unist/unist-tests.ts

typescript-bot · 2021-07-09T18:18:52Z

@wooorm Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

types/unist/index.d.ts

types/unist/unist-tests.ts

ChristianMurphy

Another consideration here, the unist and unified community use significant amount of JSDoc based TypeScript, where generics can be a bit more ackward and conferencing is leaned on more heavily than TypeScript with TS-syntax.

Could we add some test cases for inferencing?
Something like

function exampleNodeUtil(node: Node) {}

const inferredNode = { type: 'example' }
const inferredNotNode = { notType: 'whoops' }

exampleNodeUtil(inferredNode)
// $ExpectError
exampleNodeUtil(inferredNotNode)

for Node, Literal, and Parent?

typescript-bot · 2021-07-09T19:14:32Z

@ChristianMurphy, @wooorm Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

JounQin · 2021-07-09T19:19:11Z

Could we add some test cases for inferencing?
Something like

@ChristianMurphy Done.

types/unist/index.d.ts

typescript-bot · 2021-07-09T19:31:23Z

@wooorm Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

types/unist/unist-tests.ts

typescript-bot · 2021-07-09T19:50:43Z

@wooorm, @ChristianMurphy Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

ChristianMurphy

Thanks @JounQin!

wooorm · 2021-07-13T18:29:39Z

@remcohaszing is the only other person who hasn’t approved or rejected, so maybe he has more thoughts.

GuiltyDolphin · 2021-07-13T22:11:51Z

I’m not opposed to this change. I don’t see a reason to add it either.

So 🤷‍♂️ I’m abstaining from approving/rejecting this change. Other people have approved it though.

I'm of the same mind - I'm not convinced the change really adds anything that can't already be achieved by using extending interfaces (which may actually be simpler/cleaner to use in general), but it doesn't break anything (functional) AFAICS... So yeah, up to @remcohaszing at this point I think.

Having said that, this does introduce the issue with error messages I mentioned before (the main one that would be encountered is probably that lots of default parameters would start showing in types/error messages), which could be a potential downside. I don't know how long it'd take for error messages to get fixed though, and that doesn't really affect the functionality.

remcohaszing · 2021-07-14T12:15:50Z

I agree with @wooorm and @GuiltyDolphin.

Earlier I had ideas very similar to this PR. However, the same goal can be achieved by extending Node or Parent.

Because of the more verbose error messages I’m thinking it might be better not to merge this.

JounQin · 2021-07-14T12:37:32Z

@remcohaszing Extending Node or Parent can still be used in lib definitions, but the current changes are useful for end users for simple usage as I described at #54413 (comment).

remcohaszing · 2021-07-14T19:05:10Z

@remcohaszing Extending Node or Parent can still be used in lib definitions, but the current changes are useful for end users for simple usage as I described at #54413 (comment).

That’s true. I’m not necessarily for or against these changes. Feel free to merge.

GuiltyDolphin · 2021-07-14T20:23:34Z

Yeah we can see how this goes and what it does to the error messages, might be fine.

JounQin · 2021-07-15T00:19:05Z

OK, then I'm going to merge.

JounQin · 2021-07-15T00:19:15Z

Ready to merge

typescript-bot · 2021-07-15T00:32:01Z

I just published @types/unist@2.0.6 to npm.

wooorm · 2021-07-15T07:19:40Z

This is breaking stuff like: https://github.com/unifiedjs/unified/runs/3074196319?check_suite_focus=true#step:5:12

JounQin · 2021-07-15T07:24:43Z

@wooorm I think that is due to

Use any[] instead of [Record<string, unknown>?] for the default plugin type parameters

The error is coming from type-coverage.

wooorm · 2021-07-15T07:25:38Z

Close, it’s because Literal defaulted to unknown, and it now defaults to any: unifiedjs/unified@75c8293

JounQin · 2021-07-15T07:27:08Z

Close, it’s because Literal defaulted to unknown, and it now defaults to any: unifiedjs/unified@75c8293

Literal still defaults to unknown

https://github.com/DefinitelyTyped/DefinitelyTyped/pull/54413/files#diff-7d07858715e69cbbd1167662a4db927ed5c920eb10e79e4e66976874806e554bR112

wooorm · 2021-07-15T07:30:24Z

Weird. My editor shows Literal<any, Data> if I use Literal. So then where’s that any coming from? 🤔

wooorm · 2021-07-15T07:30:49Z

This PR: TData extends object. Isn’t object just any?

I think the lowercase object in this PR are anys

JounQin · 2021-07-15T07:32:37Z

TData extends object. Isn’t object just any?

No, of course not. It means a true Object. string extends Object === true while string extends object === false

JounQin · 2021-07-15T07:34:58Z

There is a test case:

// $ExpectError
type DataType = NodeData<Node<string>>;

Because string does not extends object.

JounQin · 2021-07-15T07:35:51Z

Weird. My editor shows Literal<any, Data> if I use Literal. So then where’s that any coming from? 🤔

Maybe a bug of TypeScript itself?

JounQin · 2021-07-15T07:44:10Z

@wooorm Maybe you can try // @ts-check at first line in JS files.

I believe it will produce better .d.ts files if you follow all ts suggestions with // @ts-check.

wooorm · 2021-07-15T07:49:45Z

All the code in unifiedjs/unified is checked already: https://github.com/unifiedjs/unified/blob/2323d38e2c74708f5a453ebfea42f80e0454a435/tsconfig.json#L9.

This might indeed be a bug with TS checking JS. In typescript the value does become unknown.

The reason I don’t use object is because of typescript-eslint/typescript-eslint#848.

JounQin · 2021-07-15T07:57:30Z

@wooorm See #54413 (comment)

Record<string, unknown> is worse than object IMO.

GuiltyDolphin · 2021-07-15T10:49:20Z

This PR: TData extends object. Isn’t object just any?

I think the lowercase object in this PR are anys

@wooorm any basically just turns of type-checking, it's best avoided where possible. unknown is for when you don't know what type you're working with/can work with any value. Record<string, unknown> and object are different (object does not extend Record<string, unknown>, but Record<string, unknown> extends object).

Quick example of any vs unknown:

// similar for `object` -- doesn't extend `number`
function size(x: unknown[]): number {
  return x[0]; // error - should be x.length
}

function size(x: any[]): number {
  return x[0]; // okay (but bad)
}

In the first case, the typechecker is saying "Oh, we aren't allowed to make any assumptions about x's type here (without narrowing), so we don't know this is a number, so this is bad." In the second case, the typechecker is saying "I'm not even going to check whether this operation is okay, because you told me using any that you can do anything you want with this value".

wooorm · 2021-07-15T14:10:23Z

Apparently, when checking JS, TS does see object as any. Hence why this PR is breaking some stuff such as #54413 (comment).
And I do prefer Record<string, unknown>, due to typescript-eslint reasoning: #54413 (comment).

JounQin · 2021-07-15T15:31:20Z

@wooorm I don't think so.

As you said Literal results Literal<any, Data> in js, the any is Value, not TData, so unkown is accidentally mapped to any in js. That should be a bug of TypeScript of course, but nothing related to object.

And for object, it is safer than index signature Record<string, unknown>:

interface A {
  key: string
}

type B<T extends Record<string, unknown> = T

type C = B<A> // error, A doesn't have index signature

type D<T extends object> = T

type E = D<A> // no error

So I disagree with typescript-eslint about object.

Generics were added in DefinitelyTypedGH-54413. This commit undoes generics. I was neutral on generics 2 years ago, but now that our entire ecosystem is typed, I am against. There were three generics: * `children` and `value`: unist is supposed to be an abstract interface that has to be extended before being used. *Those* types are supposed to choose a `type` field, specify which other fields exist, and specify particular children or value. * `data`: I don’t think generic data is a good idea. `Data` is a *shared* space that *several* plugins/utilities can access to store whatever they want. A generic makes it possible to hide conflicts. Generics also hide what is actually supported. We’ve been adding an alternative in other places (micromark, vfile, mdast/hast/etc for content types of nodes), and I think we should do the same for data: use an interface that can be registered onto. While data is not useful on `unist`, I plan to do the same to `mdast` and `hast`, where it gets very useful: `mdast-util-to-hast` for example sets a [`meta` field on `hast`s `element.data`](https://github.com/syntax-tree/mdast-util-to-hast/blob/c87cd60673/lib/handlers/code.js#L41), and it adds support for `hProperties`/`hChildren`/`hName` on all mdast nodes. If that utility registers with `hast` `Element.Data` and `mdast` `Node.data`, all other utilities and plugins can get typed data. Lastly, I searched GH for `unist` and `Node<` and did not see much use, expect for ugly generated types, such as: https://github.com/nachoaldamav/hardlinks-test/blob/f40b5ca/data/unist-util-map/index.d.ts#L33.

typescript-bot added Edits Owners This PR adds or removes owners Critical package labels Jul 9, 2021

JounQin commented Jul 9, 2021

View reviewed changes

types/unist/index.d.ts Show resolved Hide resolved

typescript-bot added the The CI failed When GH Actions fails label Jul 9, 2021

typescript-bot added The CI failed When GH Actions fails and removed The CI failed When GH Actions fails labels Jul 9, 2021

typescript-bot added Edits multiple packages and removed The CI failed When GH Actions fails labels Jul 9, 2021

wooorm reviewed Jul 9, 2021

View reviewed changes

JounQin mentioned this pull request Jul 9, 2021

feat: bump all upgradable deps, fix related usage mdx-js/eslint-mdx#321

Merged

ChristianMurphy reviewed Jul 9, 2021

View reviewed changes

types/unist/index.d.ts Outdated Show resolved Hide resolved

types/unist/index.d.ts Outdated Show resolved Hide resolved

types/unist/unist-tests.ts Show resolved Hide resolved

typescript-bot removed the Edits multiple packages label Jul 9, 2021

ChristianMurphy suggested changes Jul 9, 2021

View reviewed changes

ChristianMurphy reviewed Jul 9, 2021

View reviewed changes

types/unist/index.d.ts Outdated Show resolved Hide resolved

types/unist/index.d.ts Outdated Show resolved Hide resolved

ChristianMurphy approved these changes Jul 9, 2021

View reviewed changes

typescript-bot added the Other Approved This PR was reviewed and signed-off by a community member. label Jul 9, 2021

wooorm reviewed Jul 9, 2021

View reviewed changes

types/unist/unist-tests.ts Outdated Show resolved Hide resolved

types/unist/unist-tests.ts Outdated Show resolved Hide resolved

typescript-bot removed the Other Approved This PR was reviewed and signed-off by a community member. label Jul 9, 2021

ChristianMurphy approved these changes Jul 9, 2021

View reviewed changes

typescript-bot merged commit df6c94b into DefinitelyTyped:master Jul 15, 2021

JounQin deleted the feat/unist_generic branch July 15, 2021 00:19

MichaelDeBoey mentioned this pull request Jul 15, 2021

CI is failing because of updated unist types remark-embedder/core#29

Closed

JounQin mentioned this pull request Jul 17, 2022

Add invoice for @JounQin unifiedjs/collective#56

Merged

5 tasks

wooorm mentioned this pull request Jun 15, 2023

types/unist: Change unist types #65789

Merged

8 tasks

Conversation

JounQin commented Jul 9, 2021

Uh oh!

typescript-bot commented Jul 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1 package in this PR

Code Reviews

Status

Uh oh!

typescript-bot commented Jul 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

typescript-bot commented Jul 9, 2021

Uh oh!

GuiltyDolphin commented Jul 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JounQin commented Jul 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Jul 9, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

typescript-bot commented Jul 9, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ChristianMurphy left a comment

Choose a reason for hiding this comment

Uh oh!

typescript-bot commented Jul 9, 2021

Uh oh!

JounQin commented Jul 9, 2021

Uh oh!

Uh oh!

Uh oh!

typescript-bot commented Jul 9, 2021

Uh oh!

Uh oh!

Uh oh!

typescript-bot commented Jul 9, 2021

Uh oh!

ChristianMurphy left a comment

Choose a reason for hiding this comment

Uh oh!

wooorm commented Jul 13, 2021

Uh oh!

GuiltyDolphin commented Jul 13, 2021

Uh oh!

remcohaszing commented Jul 14, 2021

Uh oh!

JounQin commented Jul 14, 2021

Uh oh!

remcohaszing commented Jul 14, 2021

Uh oh!

GuiltyDolphin commented Jul 14, 2021

Uh oh!

JounQin commented Jul 15, 2021

Uh oh!

JounQin commented Jul 15, 2021

Uh oh!

typescript-bot commented Jul 15, 2021

Uh oh!

wooorm commented Jul 15, 2021

Uh oh!

JounQin commented Jul 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wooorm commented Jul 15, 2021

Uh oh!

JounQin commented Jul 15, 2021

Uh oh!

wooorm commented Jul 15, 2021

typescript-bot commented Jul 9, 2021 •

edited

Loading

typescript-bot commented Jul 9, 2021 •

edited

Loading

GuiltyDolphin commented Jul 9, 2021 •

edited

Loading

JounQin commented Jul 9, 2021 •

edited

Loading

JounQin commented Jul 15, 2021 •

edited

Loading

wooorm commented Jul 15, 2021 •

edited

Loading

GuiltyDolphin commented Jul 15, 2021 •

edited

Loading