unifiedjs / unified

☔️ interface for parsing, inspecting, transforming, and serializing content through syntax trees
https://unifiedjs.com
MIT License
4.49k stars 110 forks source link

3rd overload signature of `FrozenProcessor.run` should accept `ParseTree` instead of `CompileTree` #173

Closed alvinleung1996 closed 3 years ago

alvinleung1996 commented 3 years ago

Initial checklist

Affected packages and versions

unified 10.1.0

Link to runnable example

No response

Steps to reproduce

The 3rd overload signature of FrozenProcessor.run is incorrect. https://github.com/unifiedjs/unified/blob/c3ba2cd7fce33c1133b95bc81d9b0a22dba5c409/index.d.ts#L307-L321

Expected behavior

run(
    node: Specific<Node, ParseTree>,
    file?: VFileCompatible | undefined
): Promise<Specific<Node, CompileTree>>

Actual behavior

run(
    node: Specific<Node, CompileTree>,
    file?: VFileCompatible | undefined
): Promise<Specific<Node, CompileTree>>

Runtime

Node v16

Package manager

Other (please specify in steps to reproduce)

OS

Linux

Build and bundle tools

Webpack

wooorm commented 3 years ago

Why? ParseTree is the input tree. CompileTree is the output tree.

ChristianMurphy commented 3 years ago

I'd second the why question. @alvinleung1996 do you have an example where an input or output which should be supported, is not supported?

alvinleung1996 commented 3 years ago

Why? ParseTree is the input tree. CompileTree is the output tree.

For the first and second overload, ParseTree is the input tree and CompileTree is the output tree. But for the third one, CompileTree is the input tree and CompileTree is the output tree, which is inconsistent with the first two overloads and is incorrect.

https://github.com/unifiedjs/unified/blob/c3ba2cd7fce33c1133b95bc81d9b0a22dba5c409/index.d.ts#L273-L321

wooorm commented 3 years ago

Ah, I misunderstood. Thanks for clarifying! I think you’re right. Care to create a pull request solving it?

alvinleung1996 commented 3 years ago

Ah, I misunderstood. Thanks for clarifying! I think you’re right. Care to create a pull request solving it?

OK, I will create a pull request for it.