-
-
Notifications
You must be signed in to change notification settings - Fork 8
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
Refactor to improve types #7
Conversation
This comment has been minimized.
This comment has been minimized.
test.js
Outdated
@@ -22,6 +29,7 @@ test('rehype2remark()', function (t) { | |||
t.equal( | |||
unified() | |||
.use(parse, {fragment: true}) | |||
// @ts-expect-error: todo open issue. | |||
.use(rehype2remark, unified()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ChristianMurphy Any idea on how this can be fixed?
My hunch is that it could be solved by doing this: https://github.com/remarkjs/remark-rehype/blob/2b460d889893bf96324fa0ffa72b772b1e25c1b2/types/index.d.ts#L5, but in JSDoc. Any idea how to do that?
Might be something else tho
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the specific TS error thrown?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test.js:32:27 - error TS2559: Type 'Processor<Settings>' has no properties in common with type 'Options'.
32 .use(rehype2remark, unified())
~~~~~~~~~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huh, this looks the same as https://github.com/unifiedjs/unified/blob/4c927c86d3c4a484101e6ac7ece0321d0f281862/types/unified-tests.ts#L38-L41 and https://github.com/unifiedjs/unified/blob/4c927c86d3c4a484101e6ac7ece0321d0f281862/types/unified-tests.ts#L143
It should be supported without explicitly wrapping it in Plugin
🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It did pass when I removed & ((options?: Options) => Transformer)
, leaving only ((destination: Processor, options?: Options) => Transformer)
, although of course some other things failed.
I don’t think it should be |
between the two instead of &
(I tried and it didn’t work). 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI I do think this is the first JSDoc-style thing accepting maybe multiple arguments? Or at least accepting an (optional) processor as the first argument?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The overloads in unified can be misleading, they'll report on the last overload that falls through.
To get a clearer trace try
interface ExamplePluginSettings {
example: string
}
function overloadedPlugin(processorOrOptions?: ExamplePluginSettings): void
function overloadedPlugin(processorOrOptions: Processor, options?: ExamplePluginSettings): void
function overloadedPlugin(processorOrOptions?: Processor | ExamplePluginSettings, options?: ExamplePluginSettings): void {}
const testingPluginType: Plugin<[ExamplePluginSettings?] | [Processor, ExamplePluginSettings?]> = overloadedPlugin
which reports back:
Type '{ (processorOrOptions?: ExamplePluginSettings | undefined): void; (processorOrOptions: Processor<Settings>, options?: ExamplePluginSettings | undefined): void; }' is not assignable to type 'Plugin<[(ExamplePluginSettings | undefined)?] | [Processor<Settings>, (ExamplePluginSettings | undefined)?], Settings>'.
Types of parameters 'processorOrOptions' and 'settings' are incompatible.
Type '[(ExamplePluginSettings | undefined)?] | [Processor<Settings>, (ExamplePluginSettings | undefined)?]' is not assignable to type '[processorOrOptions?: ExamplePluginSettings | undefined]'.
Type '[Processor<Settings>, (ExamplePluginSettings | undefined)?]' is not assignable to type '[processorOrOptions?: ExamplePluginSettings | undefined]'.
Type at position 0 in source is not compatible with type at position 0 in target.
Property 'example' is missing in type 'Processor<Settings>' but required in type 'ExamplePluginSettings'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ChristianMurphy Figured it out, see next push.
But, what’s the reason unified.Settings
is passed as a type param to plugins? https://github.com/unifiedjs/unified/blob/45eb72e7e7bb4745b94677c5f5681ac0c433d704/types/index.d.ts#L21
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was a compromise to get untyped plugins kinda working in typescript.
If no type is provided for a plugin's settings, it assumes a single interface with something object like.
I wouldn't mind walking that back out, either requiring an options interface, or defaulting to no options, now that more of the ecosystem has types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also not sure the error has to do with the default, some examples from testing locally
function overloadedPlugin(processorOrOptions?: ExamplePluginSettings): Transformer
function overloadedPlugin(processorOrOptions: Processor, options?: ExamplePluginSettings): Transformer
function overloadedPlugin(processorOrOptions?: Processor | ExamplePluginSettings, options?: ExamplePluginSettings): Transformer {return () =>{}}
// error on next line, TS doesn't see the overload as compatible with plugin
const testing123: Plugin<[ExamplePluginSettings?]|[Processor, ExamplePluginSettings?]> = overloadedPlugin
processor.use(testing123)
function overloadedPlugin(processorOrOptions?: ExamplePluginSettings): Transformer
function overloadedPlugin(processorOrOptions: Processor, options?: ExamplePluginSettings): Transformer
function overloadedPlugin(processorOrOptions?: Processor | ExamplePluginSettings, options?: ExamplePluginSettings): Transformer {return () =>{}}
const testing123: Plugin<[ExamplePluginSettings?]> | Plugin<[Processor, ExamplePluginSettings?]> = overloadedPlugin
// error on next line, TS does consider a union of Plugins to be compatible with Plugin
processor.use(testing123)
Released in 8.1.0! |
Nice! Is the index.d.ts file supposed to be included in the release, though? 😅 |
It is #8 |
Ah! Good to know where to update package.json for next time. Thanks! |
I improved the types a bit, namely:
test.js
too, to see if they work, which leaves one bug we should fix/cc @cbush