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

Add typings #9

Conversation

ChristianMurphy
Copy link
Member

typings are based off types available in definitely typed,
and have been updated to add type predicate support,
meaning when unist-util-is is used as an if conditional, everything
inside the conditional scope can use the narrowed type.

@ChristianMurphy ChristianMurphy requested a review from a team July 21, 2019 20:56
@ChristianMurphy
Copy link
Member Author

/cc @Rokt33r

@ChristianMurphy
Copy link
Member Author

📓 NOTE: this is a breaking change, type predicates require TypeScript version 3 or higher.
Previously typings were designed for TypeScript 2.

@ChristianMurphy
Copy link
Member Author

The difference in use

// before
if (is('heading', node)) {
  const typedNode = node as Heading
  // do something
}

// after
if (is<Heading>('heading', node)) {
  // node is automatically type Heading within this scope
  // do something
}

types/index.d.ts Outdated Show resolved Hide resolved
types/index.d.ts Outdated Show resolved Hide resolved
@Rokt33r

This comment has been minimized.

typings are based off types available in definitely typed,
and have been updated to add type predicate support,
meaning when `unist-util-is` is used as an `if` conditional, everything
inside the conditional scope can use the narrowed type.

Co-authored-by: Junyoung Choi <fluke8259@gmail.com>
Co-authored-by: Junyoung Choi <fluke8259@gmail.com>
@ChristianMurphy

This comment has been minimized.

Copy link
Contributor

@Rokt33r Rokt33r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 😍 😍 😍

Copy link
Member

@wooorm wooorm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One Q: is convert.d.ts correctly picked up, when published and installed? I believe types/tsconfig.json defines paths, but it isn’t published, and package.json defines the main definitions only?

@Rokt33r
Copy link
Contributor

Rokt33r commented Jul 23, 2019

@wooorm I think I should try it. I'll let you know if it is good or not.

@Rokt33r

This comment has been minimized.

@Rokt33r
Copy link
Contributor

Rokt33r commented Jul 24, 2019

@ChristianMurphy
It seems those type testings don't work well with typescript@next(3.6.0-dev.20190724) now.

Error: /Users/junyoung/Code/unist-util-is/unist-util-is-test.ts:98:1
ERROR: 98:1   expect  TypeScript@next: Expected an error on this line, but found none.
ERROR: 109:1  expect  TypeScript@next: Expected an error on this line, but found none.
ERROR: 190:1  expect  TypeScript@next: Expected an error on this line, but found none.

I guess it should be better to do the test with the current local typescript installation. IMO, testing with unstable build feels a bit ridiculous.

If we want to do so, we need to provide --localTs option like the below.

    "test-types": "dtslint . --localTs node_modules/typescript/lib",

Co-authored-by: Junyoung Choi <fluke8259@gmail.com>
@ChristianMurphy
Copy link
Member Author

Thanks @Rokt33r!
Flattened types in this PR

@ChristianMurphy

This comment has been minimized.

@ChristianMurphy
Copy link
Member Author

@Rokt33r I tested with typescript@3.6.0-beta I am not seeing the issue.

➜  npm i -D typescript@beta
npm WARN tslint@5.14.0 requires a peer of typescript@>=2.1.0 || >=2.1.0-dev || >=2.2.0-dev || >=2.3.0-dev || >=2.4.0-dev || >=2.5.0-dev || >=2.6.0-dev || >=2.7.0-dev || >=2.8.0-dev || >=2.9.0-dev || >=3.0.0-dev || >= 3.1.0-dev || >= 3.2.0-dev but none is installed. You must install peer dependencies yourself.
npm WARN tsutils@2.29.0 requires a peer of typescript@>=2.1.0 || >=2.1.0-dev || >=2.2.0-dev || >=2.3.0-dev || >=2.4.0-dev || >=2.5.0-dev || >=2.6.0-dev || >=2.7.0-dev || >=2.8.0-dev || >=2.9.0-dev || >= 3.0.0-dev || >= 3.1.0-dev but none is installed. You must install peer dependencies yourself.
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: fsevents@1.2.9 (node_modules/fsevents):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for fsevents@1.2.9: wanted {"os":"darwin","arch":"any"} (current: {"os":"linux","arch":"x64"})

+ typescript@3.6.0-beta
updated 1 package and audited 6893 packages in 19.956s
➜ npm run test-types 

> unist-util-is@3.0.0 test-types ~/unist-util-is
> dtslint . --localTs node_modules/typescript/lib

@ChristianMurphy
Copy link
Member Author

Oddly enough, I'm also not able to reproduce the issue on the next version. 🤔

$ npx envinfo --npmPackages                 
npx: installed 1 in 1.214s

  npmPackages:
    browserify: ^16.0.0 => 16.3.0 
    dtslint: ^0.9.0 => 0.9.0 
    nyc: ^14.0.0 => 14.1.1 
    prettier: ^1.0.0 => 1.18.2 
    remark-cli: ^6.0.0 => 6.0.1 
    remark-preset-wooorm: ^5.0.0 => 5.0.0 
    tape: ^4.0.0 => 4.11.0 
    tinyify: ^2.0.0 => 2.5.1 
    typescript: ^3.6.0-dev.20190724 => 3.6.0-dev.20190724 
    unified: ^8.3.2 => 8.3.2 
    xo: ^0.24.0 => 0.24.0 


$ node --version
v12.6.0

$ npm --version
6.9.0

$ npx dtslint . --localTs node_modules/typescript/lib

Copy link
Contributor

@Rokt33r Rokt33r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ChristianMurphy It works again after I flushed ~/.dts. Maybe the build in my mac was wrong.

It looks good to be merged now. 😄

@ChristianMurphy
Copy link
Member Author

Thanks for double checking @Rokt33r 🙇‍♂️


@wooorm merging this in now, the changes to rely on TypeScript 3 will make this a SemVer major update.

@ChristianMurphy ChristianMurphy merged commit a23423c into syntax-tree:master Jul 25, 2019
@ChristianMurphy ChristianMurphy deleted the types/add-typescript-typings branch July 25, 2019 03:53
wooorm pushed a commit that referenced this pull request Jul 26, 2019
Closes GH-9.

Reviewed-by: Junyoung Choi <fluke8259@gmail.com>
Reviewed-by: Titus Wormer <tituswormer@gmail.com>

Co-authored-by: Junyoung Choi <fluke8259@gmail.com>
@wooorm wooorm changed the title types: add typings for unist-util-is Add typings Aug 12, 2019
@wooorm wooorm added the 💪 phase/solved Post is done label Apr 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
☂️ area/types This affects typings 💪 phase/solved Post is done 🧑 semver/major This is a change 🦋 type/enhancement This is great to have
Development

Successfully merging this pull request may close these issues.

3 participants