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

Rewrite types #150

Merged
merged 6 commits into from
Jul 15, 2021
Merged

Rewrite types #150

merged 6 commits into from
Jul 15, 2021

Conversation

wooorm
Copy link
Member

@wooorm wooorm commented Jul 13, 2021

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and couldn’t find anything (or linked relevant results below)
  • If applicable, I’ve added docs and tests

Description of changes

This replaces the types.

I did a ton of work rewriting everything, but JSDoc doesn’t work well for unified. So in the end, it’s back to a manually written .d.ts file. However: TS is now checking the whole code base, and I’ve tested the types with several other unified* repos, which caught some stuff.

I did try and cut down on complexity. This mostly means removing superfluous overloads, but I’ve also removed the type parameter P for processor settings.

I don’t think P is very important. Especially now that processors like remark or rehype have less options, and users are moving towards using remark-{parse,stringify}/rehype-{parse,stringify}, which will be typed plugins like any other.
It also uses any[] instead of [Settings?] for the default plugin type parameter. In some cases plugins accept other values. While not too common, it was making those types hard. And as I’m planning on typing all the plugins, those any[]s will be gone soon.

I did catch several small bugs though, although I’ll probably catch a few more when trying to type all plugins.

edit: this used to contain a Q about two Transformer types vs one. I opted to go with one, which lets typescript assign the proper types to transformer variables. Not having to do so means Node and VFile are passed down to plugins, instead of them having to include types/unist and vfile

/cc @remcohaszing

@wooorm wooorm added the ☂️ area/types This affects typings label Jul 13, 2021
@wooorm wooorm requested a review from ChristianMurphy July 13, 2021 17:37
@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Jul 13, 2021
@@ -0,0 +1,171 @@
import {expectType, expectError} from 'tsd'
Copy link
Member Author

Choose a reason for hiding this comment

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

this is smaller because the actual tests now catch most stuff.

@codecov-commenter

This comment has been minimized.

index.d.ts Outdated
* @returns
* Current processor.
*/
use<S extends any[] = any[]>(
Copy link
Member

Choose a reason for hiding this comment

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

(optional) it could be good time to give S a more descriptive name

Suggested change
use<S extends any[] = any[]>(
use<Settings extends any[] = any[]>(

Copy link
Member

Choose a reason for hiding this comment

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

I'm cautious about defaulting to allow any number of settings of any type by default.
It may be a good default to start with no settings, and let plugins add them as needed

Suggested change
use<S extends any[] = any[]>(
use<S extends any[] = []>(

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea to make it more descriptive. Settings is also a bit vague though. I think especially the fact that it's an array, for the variadic args, is confusing and doesn't work well with Settings. What about 'PluginArguments'? 'ExpectedParameters'?

Copy link
Member

Choose a reason for hiding this comment

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

I'd lean towards PluginParameters or PluginArguments

Copy link
Member Author

Choose a reason for hiding this comment

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

On any[] => []:

  1. xo through @typescript-eslint/ban-types doesn’t allow [].
  2. It introduces a lot of errors, which I think point to valid use cases, which are now nicely written in the tests I think:
> [email protected] build
> rimraf "test/**/*.d.ts" && tsc && tsd && type-coverage

lib/index.js:138:23 - error TS2345: Argument of type 'unknown[]' is not assignable to parameter of type '[] | [boolean]'.
  Type 'unknown[]' is not assignable to type '[boolean]'.
    Target requires 1 element(s) but source may have fewer.

138       destination.use(...attachers[index])
                          ~~~~~~~~~~~~~~~~~~~

lib/index.js:193:52 - error TS2556: A spread argument must either have a tuple type or be passed to a rest parameter.

193       const transformer = attacher.call(processor, ...options)
                                                       ~~~~~~~~~~

test/use.js:103:12 - error TS2769: No overload matches this call.
  Overload 1 of 3, '(plugin: Plugin<[]>, ...settings: [] | [boolean]): Processor', gave the following error.
    Argument of type '{}[][]' is not assignable to parameter of type 'Plugin<[]>'.
      Type '{}[][]' provides no match for the signature '(this: Processor): void | Transformer'.

103       .use([
               ~
104         [
    ~~~~~~~~~
... 
123         ]
    ~~~~~~~~~
124       ])
    ~~~~~~~


test/use.js:118:15 - error TS2683: 'this' implicitly has type 'any' because it does not have a type annotation.

118               this,
                  ~~~~

  test/use.js:116:11
    116           function () {
                  ~~~~~~~~
    An outer value of 'this' is shadowed by this container.

test/use.js:217:7 - error TS2769: No overload matches this call.
  Overload 1 of 3, '(plugin: Plugin<[]>, ...settings: [] | [boolean]): Processor', gave the following error.
    Argument of type '(boolean | (() => void))[][]' is not assignable to parameter of type 'Plugin<[]>'.
      Type '(boolean | (() => void))[][]' provides no match for the signature '(this: Processor): void | Transformer'.
  Overload 2 of 3, '(tuple: [Plugin<[(boolean | (() => void))[]]>, (boolean | (() => void))[]] | [Plugin<[(boolean | (() => void))[]]>, boolean]): Processor', gave the following error.
    Type '(() => void)[]' is not assignable to type 'Plugin<[(boolean | (() => void))[]]>'.
      Type '(() => void)[]' provides no match for the signature '(this: Processor, settings_0: (boolean | (() => void))[]): void | Transformer'.
  Overload 3 of 3, '(presetOrList: Preset | PluggableList): Processor', gave the following error.
    Type 'false' is not assignable to type 'undefined'.

217       processor.use([[plugin], [plugin, false]]).freeze()
          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


test/use.js:234:9 - error TS2769: No overload matches this call.
  Overload 2 of 3, '(tuple: [Plugin<[(boolean | (() => void))[]]>, (boolean | (() => void))[]] | [Plugin<[(boolean | (() => void))[]]>, boolean]): Processor', gave the following error.
    Type '(false | (() => void))[]' is not assignable to type 'Plugin<[(boolean | (() => void))[]]>'.
      Type '(false | (() => void))[]' provides no match for the signature '(this: Processor, settings_0: (boolean | (() => void))[]): void | Transformer'.

234         [plugin, false],
            ~~~~~~~~~~~~~~~


test/use.js:251:9 - error TS2769: No overload matches this call.
  Overload 2 of 3, '(tuple: [Plugin<[(((options?: unknown) => void) | { foo: boolean; })[]]>, (((options?: unknown) => void) | { foo: boolean; })[]] | [Plugin<[(((options?: unknown) => void) | { foo: boolean; })[]]>, boolean]): Processor', gave the following error.
    Type '(false | ((options?: unknown) => void))[]' is not assignable to type 'Plugin<[(((options?: unknown) => void) | { foo: boolean; })[]]>'.
      Type '(false | ((options?: unknown) => void))[]' provides no match for the signature '(this: Processor, settings_0: (((options?: unknown) => void) | { foo: boolean; })[]): void | Transformer'.

251         [plugin, false],
            ~~~~~~~~~~~~~~~


test/use.js:402:9 - error TS2769: No overload matches this call.
  Overload 2 of 3, '(tuple: [Plugin<[]>] | [Plugin<[]>, boolean]): Processor', gave the following error.
    Argument of type '{ plugins: {}[][]; }' is not assignable to parameter of type '[Plugin<[]>] | [Plugin<[]>, boolean]'.
      Object literal may only specify known properties, and 'plugins' does not exist in type '[Plugin<[]>] | [Plugin<[]>, boolean]'.

402         plugins: [
            ~~~~~~~~~~
403           [plugin1, one],
    ~~~~~~~~~~~~~~~~~~~~~~~~~
404           [plugin2, two]
    ~~~~~~~~~~~~~~~~~~~~~~~~
405         ]
    ~~~~~~~~~


test/use.js:434:9 - error TS2769: No overload matches this call.
  Overload 2 of 3, '(tuple: [Plugin<[]>] | [Plugin<[]>, boolean]): Processor', gave the following error.
    Argument of type '{ plugins: { plugins: {}[][]; }[]; }' is not assignable to parameter of type '[Plugin<[]>] | [Plugin<[]>, boolean]'.
      Object literal may only specify known properties, and 'plugins' does not exist in type '[Plugin<[]>] | [Plugin<[]>, boolean]'.

434         plugins: [{plugins: [[plugin1, one]]}, {plugins: [[plugin2, two]]}]
            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~



Found 9 errors.

I’d say any[] is fine, which people can overwrite.

Object.assign(this, {
Parser: SimpleParser,
Compiler: SimpleCompiler
})
Copy link
Member

Choose a reason for hiding this comment

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

This is super weird. I found this hard to believe, but you’re right. This works in TypeScript, but not in type checked JavaScript 🤔

@wooorm
Copy link
Member Author

wooorm commented Jul 14, 2021

done, thank you, nice catches!

Copy link
Member

@ChristianMurphy ChristianMurphy left a comment

Choose a reason for hiding this comment

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

Thanks @wooorm!

@wooorm wooorm merged commit b3e2464 into main Jul 15, 2021
@wooorm wooorm deleted the jsdoc branch July 15, 2021 07:06
@wooorm wooorm added the 💪 phase/solved Post is done label Jul 15, 2021
@github-actions

This comment has been minimized.

@github-actions github-actions bot removed the 🤞 phase/open Post is being triaged manually label Jul 15, 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
Development

Successfully merging this pull request may close these issues.

4 participants