Add plugin tree type parameter by wooorm · Pull Request #156 · unifiedjs/unified

wooorm · 2021-07-26T09:34:23Z

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 allows plugins to define what type of tree they receive and yield in their transformer.
It does not prevent misuse of those plugins (such as a rehype plugin and then a remark plugin). Nor does it handle parser or compiler plugins.

This allows plugins to define what type of tree they can process in their transformer. It does not prevent misuse of those plugins (such as a rehype plugin and then a remark plugin). Nor does it handle parser or compiler plugins.

This allows plugins to define what type of tree they yield in their transformer. It does not prevent misuse of those plugins (such as a rehype plugin and then a remark plugin). Nor does it handle parser or compiler plugins.

wooorm · 2021-07-26T09:34:57Z

/cc @remcohaszing

ChristianMurphy

🤔 I like the idea of checking input and output AST types. 👍
In an ideal world, unified could do chained type checking, ensuring the output of a transform aligns with the input of the next transform.
parser and compiler plugins will always be a bit funky since their order is independent of transformers, but checking just the transformers could add value. 👍

This seems to be a bit different from what is described above though.
It allows Plugins to set a generic for their input and output, but this doesn't appear to be checked by unified to ensure the unified.use chain is valid?

Is this intended? (from your description it appear so?) If so, could you expand a bit on the value of adding the generics if they are not checked by unified.use?

wooorm · 2021-07-28T08:05:52Z

It allows Plugins to set a generic for their input and output, but this doesn't appear to be checked by unified to ensure the unified.use chain is valid?

Correct.

Is this intended? (from your description it appear so?) If so, could you expand a bit on the value of adding the generics if they are not checked by unified.use?

So that plugins can start defining what input/output they receive/yield. unified otherwise gives/accepts Node, which due to:

index signature removal
improved mdast/hast (with their registries)
hast / mdast utilities now accepting hast / mdast nodes rather than Node
unist-util-visit-* now searching the tree for nodes rather than assuming that they contain a <Heading>
…Node becomes annoying and too abstract.

A future idea I have:

A ParserPlugin<OutputTree> / CompilerPlugin<InputTree>:
Which could perhaps be inferred from Plugin<[], void, OutputTree> or Plugin<[], InputTree, void> respectively
or even Plugin<[], string|Buffer, OutputTree> or Plugin<[], InputTree, string|Buffer|unknown>
…which when .used stores those values on the Processor<Input, ParseTree, StringifyTree, Output>, and affects the input/output of .parse, .run, .stringify, and .process?

Perhaps this could be includes in this PR.

The last step is checking whether the .use chains are correct. But because processors are mutable, that’s quite hard:

const processor = unified()
processor.use(compilerPlugin)
processor.use(parserPlugin)
processor.use(transformPlugin)

Though, most people will probably chain?

ChristianMurphy · 2021-07-28T14:05:37Z

unist-util-visit-* now searching the tree for nodes rather than assuming that they contain a <Heading>

Follow up question on this added at syntax-tree/unist-util-visit-parents#10 (comment)

hast / mdast utilities now accepting hast / mdast nodes rather than Node

A fair point.
I agree this is a challenge, the alternative, adding is or assert type assertions to every plugin doesn't seem performant or useful.
There is a notable difference between utils and plugins though.
utils are generally directly passed a tree, TypeScript can directly check the types match.
plugins are generally called though unified, the tree being indirectly passed, without adding something to unified to try to match up the chained inputs and outputs, we would miss otherwise detectable errors.

which when .used stores those values on the Processor<Input, ParseTree, StringifyTree, Output>, and affects the input/output of .parse, .run, .stringify, and .process?

That's what I was thinking as well

const processor = unified()
processor.use(compilerPlugin)
processor.use(parserPlugin)
processor.use(transformPlugin)

Each of these .use() mutate the processor? (I didn't know that was supported)
I'd be fine not supporting changing between node types when using this particular syntax.
I do think it would still be possible to support a single tree/node type with it.

wooorm · 2021-07-28T14:34:51Z

Working on it!

wooorm · 2021-07-28T17:57:03Z

I’ve implemented parsers, compilers, etc for plugins. Now they:

define the result of .parse, .stringify, .process, .run
define the input of .run
their output defines the input of the next plugin

I have not implemented errors of some sort when misconfiguring plugins.
Given that:

It is fine to only define transform plugins on a processor because .run is used and not .parse/.stringify
several parser plugins / several compiler plugins could be used, overwriting what came before
code is already used a lot in the wild
the complexity of it all

I’m quite okay with not throwing on .use(rehypeWhatever).use(remarkWhatever)

ChristianMurphy

It is fine to only define transform plugins on a processor

I'm not sure I follow, there is a CurrentTree which I assume is the current tree, as output the most recent plugin?
We can assert that it should match the Input of the next Plugin?

several parser plugins / several compiler plugins could be used, overwriting what came before

This impacts parsers and compilers, but transformers should still be fine?

code is already used a lot in the wild

It is, but these errors would be things which never worked

the complexity of it all

Most of the complexity seems to come from conditionals to infer, would matching the generics add a lot more complexity?

index.d.ts

ChristianMurphy · 2021-07-28T23:02:53Z

index.d.ts

+type UsePlugin<
+  ParseTree extends Node,
+  CurrentTree extends Node,
+  CompileTree extends Node,
+  CompileResult,
+  Input,
+  Output
+> = Output extends Node
+  ? Input extends string
+    ? // If `Input` is `string` and `Output` is `Node`, then this plugin
+      // defines a parser, so set `ParseTree`.
+      // This also assumes that this is the first plugin (so no current tree is
+      // or compile tree is defined yet), so set those too.
+      Processor<Output, Output, Output, CompileResult>
+    : Input extends Node
+    ? // If `Input` is `Node` and `Output` is `Node`, then this plugin defines a
+      // transformer, its output defines the input of the next, so set
+      // `CurrentTree`.
+      Processor<ParseTree, Output, CompileTree, CompileResult>
+    : // Else, `Input` is something else and `Output` is `Node`:
+      never
+  : Input extends Node
+  ? // If `Input` is `Node` and `Output` is not a `Node`, then this plugin
+    // defines a compiler, so set `CompileTree` and `CompileResult`
+    Processor<ParseTree, CurrentTree, Input, Output>
+  : // Else, `Input` is not a `Node` and `Output` is not a `Node`.
+    // Maybe it’s untyped, or the plugin throws an error (`never`), so lets
+    // just keep it as it was.
+    Processor<ParseTree, CurrentTree, CompileTree, CompileResult>


interesting use of inference to extract generics.
Is this approach used to overcome quirks in JSDoc generics?
Are there other benefits?

How else could it be done?

Would Plugin<Input, Output = Input> work?
Letting TS infer when a transformer is provided, otherwise pass through the input?

that’s done in:

export type Plugin< PluginParameters extends any[] = any[], Input = Node, Output = Input > = (

And the use overloads:

use< PluginParameters extends any[] = any[], Input = Specific<Node, CurrentTree>, Output = Input >

…but this conditional makes a processor, based on how one is currently configured, and applying those Input / Output from a plugin

index.d.ts

ChristianMurphy · 2021-07-28T23:11:43Z

index.d.ts

 export type Pluggable<PluginParameters extends any[] = any[]> =
-  | PluginTuple<PluginParameters>
-  | Plugin<PluginParameters>
+  | PluginTuple<PluginParameters, any, any>
+  | Plugin<PluginParameters, any, any>


would it make sense/add value to carry the generics through this type as well, rather then anys?

yes, I’ll try!

wooorm · 2021-07-29T08:46:41Z

We can assert that it should match the Input of the next Plugin?

We could, but existing plugins can be untyped, not typed with input/output, or implicit. I think it’ll cause way to many errors. I’ll check again but what I tried yesterday resulted in 100s of errors in this code base

It is, but these errors would be things which never worked

Wrong code never worked (rehype plugin before remark plugin without rehype-remark), but I’m not worried about that.
I’m worried about this breaking all the existing plugins.

* Clean `VFileWithOutput` * Add generics to `Parser`, `Compiler` * Smarter defaults for `ParserTree`, `CurrentTree`, `CompilerTree` * Be smarter about parse/compile trees if only parse/compile plugins are used * Add smarter type of `this` in parse/transform/compile plugins. * Add docs for type parameters

wooorm · 2021-07-29T11:57:46Z

Made the types smarter.
I don’t see how to allow untyped/implicit plugins while also emitting errors when they are chained incorrectly.
I’m not sure how to infer these things from Pluggable / PluggableList / Preset though

index.d.ts

Co-authored-by: Christian Murphy <christian.murphy.42@gmail.com>

wooorm added 2 commits July 26, 2021 10:48

Add plugin tree type parameter

1ee27a5

This allows plugins to define what type of tree they can process in their transformer. It does not prevent misuse of those plugins (such as a rehype plugin and then a remark plugin). Nor does it handle parser or compiler plugins.

Add plugin output tree type parameter

09a82f8

This allows plugins to define what type of tree they yield in their transformer. It does not prevent misuse of those plugins (such as a rehype plugin and then a remark plugin). Nor does it handle parser or compiler plugins.

wooorm added ☂️ area/types This affects typings 🦋 type/enhancement This is great to have 🧒 semver/minor This is backwards-compatible change labels Jul 26, 2021

github-actions bot added the 👋 phase/new Post is being triaged automatically label Jul 26, 2021

wooorm requested review from ChristianMurphy and Murderlon July 26, 2021 09:34

github-actions bot added 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Jul 26, 2021

ChristianMurphy reviewed Jul 27, 2021

View reviewed changes

Add types for parse tree, current tree, compile tree, compile result

f41ff9f

This comment has been minimized.

Sign in to view

ChristianMurphy reviewed Jul 28, 2021

View reviewed changes

wooorm added 2 commits July 29, 2021 14:00

Remove unneeded things

6de2db1

Revert

7d4279f

ChristianMurphy approved these changes Jul 30, 2021

View reviewed changes

index.d.ts Outdated Show resolved Hide resolved

index.d.ts Outdated Show resolved Hide resolved

index.d.ts Outdated Show resolved Hide resolved

index.d.ts Outdated Show resolved Hide resolved

ChristianMurphy requested a review from a team July 30, 2021 01:48

wooorm and others added 2 commits July 30, 2021 09:34

Apply suggestions from code review

c6dcc5d

Co-authored-by: Christian Murphy <christian.murphy.42@gmail.com>

typos

e333401

wooorm merged commit 134ecad into main Jul 30, 2021

wooorm deleted the plugin-tree branch July 30, 2021 10:00

wooorm added the 💪 phase/solved Post is done label Jul 30, 2021

This comment has been minimized.

Sign in to view

github-actions bot removed the 🤞 phase/open Post is being triaged manually label Jul 30, 2021

Uh oh!

Conversation

wooorm commented Jul 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Initial checklist

Description of changes

Uh oh!

wooorm commented Jul 26, 2021

Uh oh!

ChristianMurphy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wooorm commented Jul 28, 2021

Uh oh!

ChristianMurphy commented Jul 28, 2021

Uh oh!

wooorm commented Jul 28, 2021

Uh oh!

This comment has been minimized.

wooorm commented Jul 28, 2021

Uh oh!

ChristianMurphy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ChristianMurphy Jul 28, 2021

Choose a reason for hiding this comment

Uh oh!

wooorm Jul 29, 2021

Choose a reason for hiding this comment

Uh oh!

ChristianMurphy Jul 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wooorm Jul 30, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ChristianMurphy Jul 28, 2021

Choose a reason for hiding this comment

Uh oh!

wooorm Jul 29, 2021

Choose a reason for hiding this comment

Uh oh!

wooorm commented Jul 29, 2021

Uh oh!

wooorm commented Jul 29, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

wooorm commented Jul 26, 2021 •

edited

Loading

ChristianMurphy left a comment •

edited

Loading

ChristianMurphy Jul 30, 2021 •

edited

Loading