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

Add plugin tree type parameter #156

Closed wooorm closed 3 years ago

wooorm commented 3 years ago

Initial checklist

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.

wooorm commented 3 years ago

/cc @remcohaszing

wooorm commented 3 years ago

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:


A future idea I have:

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 commented 3 years ago

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

Follow up question on this added at https://github.com/syntax-tree/unist-util-visit-parents/pull/10#issuecomment-888310388

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 commented 3 years ago

Working on it!

codecov-commenter commented 3 years ago

Codecov Report

Merging #156 (e333401) into main (cf8653b) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #156   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines          600       600           
=========================================
  Hits           600       600           

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update cf8653b...e333401. Read the comment docs.

wooorm commented 3 years ago

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

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

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

wooorm commented 3 years ago

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.

wooorm commented 3 years ago

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

github-actions[bot] commented 3 years ago

Hi! This was closed. Team: If this was merged, please describe when this is likely to be released. Otherwise, please add one of the no/* labels.