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

types: revist transformer typescript interface #65

Closed
ChristianMurphy opened this issue Jul 20, 2019 · 1 comment
Closed

types: revist transformer typescript interface #65

ChristianMurphy opened this issue Jul 20, 2019 · 1 comment
Labels
🤷 no/invalid This cannot be acted upon 💬 type/discussion This is a request for comments

Comments

@ChristianMurphy
Copy link
Member

Subject of the feature

Update the Transformer interface to better reflect the types being passed in.

Problem

Transformer expects to receive exactly a unist Node

unified/types/index.d.ts

Lines 296 to 316 in e408747

/**
* Transformers modify the syntax tree or metadata of a file. A transformer is a function which is invoked each time a file is passed through the transform phase.
* If an error occurs (either because it’s thrown, returned, rejected, or passed to `next`), the process stops.
*
* The transformation process in unified is handled by `trough`, see it’s documentation for the exact semantics of transformers.
*
* @param node Node or tree to be transformed
* @param file File associated with node or tree
* @param next If the signature of a transformer includes `next` (third argument), the function may finish asynchronous, and must invoke `next()`.
* @returns
* - `Error` — Can be returned to stop the process
* - `Node` — Can be returned and results in further transformations and `stringify`s to be performed on the new tree
* - `Promise` — If a promise is returned, the function is asynchronous, and must be resolved (optionally with a `Node`) or rejected (optionally with an `Error`)
*/
interface Transformer {
(
node: Node,
file: VFile,
next?: (error: Error | null, tree: Node, file: VFile) => {}
): Error | Node | Promise<Node>
}

When in most cases parsers will return a RootNode which extends ParentNode which adds an extra children attribute that Node does not expect.

remark ran into a similar issue to this with the visitor interface, see remarkjs/remark#426

Expected behaviour

Ideally avoid requiring as casting the node parameter.

Alternatives

  1. do nothing, casting may be an okay option
  2. use a generic <T extends Node> to guarantee the incoming node is a super type of Node, though it may or may not reflect the correct super type.
  3. Allow Processor to accept a generic of all possible types (E.G. all mdast nodes) and pass that generic type union in (Open question: How would plugins add new types to the union?)
  4. Other ideas?
@ChristianMurphy ChristianMurphy added 🙋 no/question This does not need any changes 🦋 type/enhancement This is great to have 🧒 semver/minor This is backwards-compatible change needs pr labels Jul 20, 2019
@ChristianMurphy
Copy link
Member Author

After trying out unist-util-visit with type predicates.
I'm leaning towards not changing this.
unist-util-visit and unist-util-is (along with the many other utilities) can provide easy ways to manipulate the Node/tree without sacrificing type safety.

@ChristianMurphy ChristianMurphy added 🤷 no/invalid This cannot be acted upon and removed needs pr labels Jul 22, 2019
ChristianMurphy added a commit to syntax-tree/unist-util-visit that referenced this issue Jul 31, 2019
related to: unifiedjs/unified#65
related to: syntax-tree/unist-util-visit-parents#7
resolves #15

* types: add typings and type tests for unist-util-visit

* types: drop unused generic

* test: add test for array of tests passed to visit

* style: format type config files

* types: leverage unist-util-is helpers

* test: refactor examples to handle unknown type for node param

* build: update dependency on unist-util-is to version 4

* types: leverage helper types from unist util visit parents, add actions

* build: leverage stable unist-util-visit-parents 3.0.0 release
wooorm pushed a commit to syntax-tree/unist-util-visit that referenced this issue Jul 31, 2019
Related-to: unifiedjs/unified#65.
Related-to: syntax-tree/unist-util-visit-parents#7.
Closes GH-15.

Reviewed-by: Junyoung Choi <fluke8259@gmail.com>
Reviewed-by: Titus Wormer <tituswormer@gmail.com>
@wooorm wooorm added 💬 type/discussion This is a request for comments and removed 🙋 no/question This does not need any changes 🦋 type/enhancement This is great to have 🧒 semver/minor This is backwards-compatible change labels Aug 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤷 no/invalid This cannot be acted upon 💬 type/discussion This is a request for comments
Development

No branches or pull requests

2 participants