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 type for rules' callback parameter #283

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jablko
Copy link
Contributor

@jablko jablko commented Mar 3, 2022

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

Transformers have a next parameter that was missing from the Rule type, so rules that use it don't type check.

@github-actions github-actions bot added the 👋 phase/new Post is being triaged automatically label Mar 3, 2022
@github-actions

This comment has been minimized.

@codecov-commenter

This comment was marked as resolved.

jablko added a commit to jablko/remark-validate-links that referenced this pull request Mar 3, 2022
| [boolean | Label | Severity, Settings],
Tree
>
): Plugin<[(Settings | [Label | Severity, Settings?] | void)?], Tree>
Copy link
Contributor Author

@jablko jablko Mar 3, 2022

Choose a reason for hiding this comment

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

To avoid getting:

lib/index.js:89:62 - error TS2345: Argument of type '[void | Options]' is not assignable to parameter of type 'void[] | [Options | Label | Severity] | [boolean | Label | Severity, Options]'.
  Type '[void | Options]' is not assignable to type '[Options | Label | Severity]'.
    Type 'void | Options' is not assignable to type 'Options | Label | Severity'.
      Type 'void' is not assignable to type 'Options | Label | Severity'.

calling lintRule(...).call(this, rawOptions) here.

I think the correct type is [(Settings | [Label | Severity, Settings?])?] (no void), however the inferred type here becomes [Settings | [Label | Severity, Settings | undefined] | undefined], which requires too many elements.

I tried with TS 4.6 and --exactOptionalPropertyTypes but couldn't get it to infer the optional elements ([(Settings | [Label | Severity, Settings?])?]). I haven't yet investigated further, whether this corresponds to an open TS issue.

I suspect this is the reason that | void[] was there before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't find an existing TS issue for the optional parameters, so I opened one: microsoft/TypeScript#48132

Copy link
Member

@wooorm wooorm Mar 7, 2022

Choose a reason for hiding this comment

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

Array<void> indeed tries to match the case where no arguments are given. The reason is that [] is used for tuples. In practise, amongst most plugins, I’ve had the most luck using Array<void>.

Your changes here miss the boolean case and IMO are a harder to read than before (because it’s unclear what the ? means, is it nullish? does it allow an explicit undefined?).
Am I correct that without these changes, you got the error you mentioned after “To avoid getting:”? Can we perhaps solve that another way?

Copy link
Contributor Author

@jablko jablko Mar 7, 2022

Choose a reason for hiding this comment

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

I think void needs to be inside the tuple: [... | void] vs. [...] | Array<void>. Logically [... | void] is assignable to [...] | Array<void> (because you can't have a heterogeneous 1-tuple) but TS doesn't support it, currently: microsoft/TypeScript#30895

I'm calling lintRule(...).call(this, rawOptions), where rawOptions is Options | void.

Plugins aren't called with boolean? https://github.com/unifiedjs/unified/blob/2323d38e2c74708f5a453ebfea42f80e0454a435/lib/index.js#L127-L133

processor.use(plugin[, options]) adds the boolean type, as it should: https://github.com/unifiedjs/unified/blob/6b060c2a229049e1bc0e7ea51920b36efb069f9f/index.d.ts#L136

? allows an explicit undefined if --exactOptionalPropertyTypes is off, otherwise not. TS currently allows an explicit undefined argument regardless, however, probably because it uses one signature for callers and for the function body? Dropping | undefined in the body would be wrong if an optional argument were omitted. The docs agree: https://www.typescriptlang.org/docs/handbook/2/functions.html#optional-parameters

Pondering out loud: Maybe TS could drop | undefined from function overloads, since those are only for callers, currently? It doesn't, maybe in case that changes in the future?

Here are playground links with and without --exactOptionalPropertyTypes:

// @exactOptionalPropertyTypes

let options: ["optional"?];
options = ["optional"]; // ✔️ No errors, as expected
options = []; // ✔️ Element is optional
options = [undefined]; // ❌ undefined isn't assignable to "optional"

function plugin(...options: ["optional"?]): void; // Test overload signature
function plugin(...options: ["optional"?]) {}
plugin("optional"); // ✔️ No errors, as expected
plugin(); // ✔️ Parameter is optional
plugin(undefined); // undefined is allowed, regardless

Copy link
Member

Choose a reason for hiding this comment

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

I think void needs to be inside the tuple:

I’ve had troubles with trying to type plugins that way, and calling them without arguments.
Only Array<void> seems to allow all cases, including no parameter.

I'm calling lintRule(...).call(this, rawOptions), where rawOptions is Options | void.

Where? This PR does not include that code. Perhaps you can, on the using side, make sure that void is removed from that value, e.g.:

if (rawOptions) {
  // ...
}

Plugins aren't called with boolean?

You are correct. Though the intent is that it’s about an array in the tuple:

.use(someRule, true) // this one is not needed, indeed
.use(someRule, [true]) // this one *is* needed, though
.use(someRule, [true, 72]) // same

? allows an explicit undefined if --exactOptionalPropertyTypes is off

If possible, I’d prefer to code explicitly in a way that is not affected by such options.
I’m not sure about others but I find it hard to write code in different projects if I have to check what it actually means by looking at the tsconfig? And I don’t expect other contributors to be aware of such small but important details.
That’s why the code is currently explicitly using fixed-size tuples. And when those aren’t possible (0-size tuple), an Array<void>.

Pondering out loud: Maybe TS could drop | undefined from function overloads

I don’t know enough about TS to decide what the right decision for them is!

Copy link
Contributor Author

@jablko jablko Mar 8, 2022

Choose a reason for hiding this comment

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

If possible, I’d prefer to code explicitly in a way that is not affected by such options.
I’m not sure about others but I find it hard to write code in different projects if I have to check what it actually means by looking at the tsconfig? And I don’t expect other contributors to be aware of such small but important details.

👍 That's smart, how about explicitly adding undefined to the optional elements? i.e. [("optional" | undefined)?]
That's the same with and without --exactOptionalPropertyTypes.

I’ve had troubles with trying to type plugins that way, and calling them without arguments.
Only Array<void> seems to allow all cases, including no parameter.

I see what you mean when they aren't optional. When they are optional, it isn't needed anymore.

.use(someRule, [true]) // this one *is* needed, though
.use(someRule, [true, 72]) // same

Are these cases supported?

Where? This PR does not include that code. Perhaps you can, on the using side, make sure that void is removed from that value, e.g.:

if (rawOptions) {
  // ...
}

Here's the using side: https://github.com/remarkjs/remark-validate-links/pull/67/files#diff-92bbac9a308cd5fcf9db165841f2d90ce981baddcb2b1e26cfff170929af3bd1R93

There, as here, if we can avoid introducing void in the first place, that's the simplest and least error-prone way to remove void?

@github-actions github-actions bot added 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Mar 3, 2022
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.

Good idea, generally, on allowing next callback!

| [boolean | Label | Severity, Settings],
Tree
>
): Plugin<[(Settings | [Label | Severity, Settings?] | void)?], Tree>
Copy link
Member

@wooorm wooorm Mar 7, 2022

Choose a reason for hiding this comment

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

Array<void> indeed tries to match the case where no arguments are given. The reason is that [] is used for tuples. In practise, amongst most plugins, I’ve had the most luck using Array<void>.

Your changes here miss the boolean case and IMO are a harder to read than before (because it’s unclear what the ? means, is it nullish? does it allow an explicit undefined?).
Am I correct that without these changes, you got the error you mentioned after “To avoid getting:”? Can we perhaps solve that another way?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤞 phase/open Post is being triaged manually
Development

Successfully merging this pull request may close these issues.

3 participants