syntax-tree / unist-util-visit Goto Github PK

Hi, currently the type for Visitor is

  type Visitor<V extends Node> = (
    node: V,
    index: number,
    parent: Node
  ) => void | Action | Index | ActionTuple

I think the parent parameter should be of type Parent, as the type Node does not have a children property.

Subject of the feature

Add Typescript typings for unist-util-visit

Problem

projects have to // @ts-ignore or manually add typings

Expected behaviour

Typings should be provided

Notes

This plugin is one of the more common utils used in transforms, the typings here can help inform a decision on unifiedjs/unified#65

This may be able to leverage https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/unist-util-is/index.d.ts

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)

Affected packages and versions

[email protected]

Link to runnable example

No response

Steps to reproduce

After upgrading from 4.1.1 to 4.1.2 I receive the errors below:

[email protected] build:esm
tsc

node_modules/unist-util-visit/lib/index.d.ts:32:24 - error TS2307: Cannot find module 'unist-util-is/lib/index.js' or its corresponding type declarations.

32 Check extends import('unist-util-is/lib/index.js').Test
~~~~~~~~~~~~~~~~~~~~~~~~~~~~

node_modules/unist-util-visit/lib/index.d.ts:115:24 - error TS2307: Cannot find module 'unist-util-is/lib/index.js' or its corresponding type declarations.

115 Check extends import('unist-util-is/lib/index.js').Test
~~~~~~~~~~~~~~~~~~~~~~~~~~~~

node_modules/unist-util-visit/lib/index.d.ts:132:24 - error TS2307: Cannot find module 'unist-util-is/lib/index.js' or its corresponding type declarations.

132 Check extends import('unist-util-is/lib/index.js').Test = string

Expected behavior

No type declaration errors.

Actual behavior

Receive type declaration errors.

Affected runtime and version

[email protected]

Affected package manager and version

[email protected]

Affected OS and version

macos

Build and bundle tools

No response

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)

Affected packages and versions

[email protected]

Link to runnable example

https://codesandbox.io/s/react-markdown-debug-forked-sqvof

Steps to reproduce

1. Open the codesandbox link
2. Inspect the TypeScript error at line 25

Expected behavior

The visit method should be able to narrow the type of directiveVisitor and not throw an error.

Actual behavior

The visit method's type is not able narrow the type of directiveVisitor because it can't read the test ['textDirective', 'leafDirective', 'containerDirective']

Runtime

Node v14

Package manager

yarn v1

OS

macOS

Build and bundle tools

Next.js

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)

Affected packages and versions

4.1.0

Link to runnable example

No response

Steps to reproduce

clone https://github.com/cjroebuck/next-build-stalled

npm i
npx tsc

typescript version 4.5.2

(typescript version 4.4.3 does build ok)

Expected behavior

build shouldn't stall

Actual behavior

build stalls indefinitely

In order to get the build to pass, comment out this line

I suspect it's an issue with the unist-util-visit types?

Runtime

Node v16

Package manager

npm v7

OS

macOS

Build and bundle tools

Next.js

Hey, @wooorm 👋

I'm working on Gatsby plugin for extending code block functionality and it's using the below syntax:

```js:title=index.js&copy=true
console.log('Some code goes here...')
```

The problem I'm facing is that &copy gets encoded to ©. Is there any way to avoid it?

This is a doc suggestion + a more general question.

It may be worth putting in the docs some thoughts on what the contract is wrt mutating the tree. In particular, modifying individual nodes is fine, but unist-util-visit assumes the visitor does not mutate the number of children of the parent.

This is a bit of a pitfall, as it's not uncommon to want to traverse and mutate adjacent children. For example, see https://github.com/jackycute/remark-html-emoji-image/blob/master/index.js#L66-L69 — but it appears that approach is buggy, since unist-util-visit visits children sequentially by index.

More generally, I'd be interested to know what approach is best to handle the fully mutating use case. https://github.com/syntax-tree/unist-util-map and https://github.com/eush77/unist-util-filter are listed but cover different use cases. One easy option (but may not be the best) is to make a visit-dynamic variant, something like:

'use strict';

module.exports = visit;

/**
 * A mutating version of https://github.com/syntax-tree/unist-util-visit
 *
 * Traverse a Unist tree, while also allowing addition/deletion of children.
 * Visitor should return number of nodes added/deleted (positive for addition, negative for removal,
 * 0 if only visited), or false to terminate. To avoid confusion, we don't support reverse traversal.
 */
function visit(tree, type, visitor) {
  if (typeof type === 'function') {
    visitor = type;
    type = null;
  }

  one(tree);

  /* Visit a single node. */
  function one(node, index, parent) {
    let result;

    index = index || (parent ? 0 : null);

    if (!type || node.type === type) {
      result = visitor(node, index, parent || null);
    }

    if (node.children && result !== false) {
      all(node);
    }

    return result;
  }

  /* Visit children in `parent`, possibly adding or removing children. */
  function all(parent) {
    let child;
    let index = 0;
    while (index < parent.children.length) {
      child = parent.children[index];

      let delta = 0;
      if (child) {
        delta = one(child, index, parent);
        if (delta === false) {  // Abort.
          return;
        }
      }

      index += 1 + (delta || 0);
    }
  }
}

Subject of the feature

I think it would be easier to get started with this if the README contained the docs about common things like returning an action instead of referring to unist-util-visit-parents.

Problem

It took me a little while to figure out that, like unist-util-visit-parents, this accepts returning an action (such as visit.SKIP).

Alternatives

Removing the details of how to call the function and just linking to unist-util-visit-parents might work.

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)

The example in the readme on main uses import, so it seems like just a new publish is needed.

Is it possible to modify nodes in an immutable way? eg,

// Redact all text nodes
node = visit(node, (child) => {
  if (child.type === 'text') {
    return { ...child, value: '[redacted]' }
  }
})

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)

Affected packages and versions: latest

Steps to reproduce

Install in Node 10 environment (I haven't tested on other node versions). Receive the following error:

Link to code example: https://github.com/googleapis/release-please/pull/870/checks?check_run_id=2390814891

Expected behavior

Library compiles.

I believe the goal is perhaps a dual-mode module (that would work for ESM and CJS?). This might just take some fiddling with extensions and export maps.

Here's what we landed on for yargs:

https://github.com/yargs/yargs/blob/master/package.json#L5

It's ugly but works on most Node versions.

Actual behavior

Error [ERR_REQUIRE_ESM]: Must use import to load ES Module: /home/runner/work/release-please/release-please/node_modules/unist-util-visit/index.js
require() of ES modules is not supported.
require() of /home/runner/work/release-please/release-please/node_modules/unist-util-visit/index.js from /home/runner/work/release-please/release-please/build/src/util/to-conventional-changelog-format.js is an ES module file as it is a .js file whose nearest parent package.json contains "type": "module" which defines all .js files in that package scope as ES modules.
Instead rename index.js to end in .cjs, change the requiring code to use import(), or remove "type": "module" from /home/runner/work/release-please/release-please/node_modules/unist-util-visit/package.json.

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)

Affected packages and versions

unist-util-visit

Link to runnable example

No response

Steps to reproduce

In following code cannot find types for tagName, properties and children.

const result = await unified()
    .use(remarkParse, { fragment: true })
    .use(remarkRehype)
    .use(rehypeSlug)
    .use(() => {
      return (tree) => {
        console.log(tree);
        visit(tree, 'element', (node) => {
          // @ts-ignore
          if (node.tagName == 'h2') {
            // console.log(node);
            // @ts-ignore
            const id = node.properties.id;
            // @ts-ignore
            const heading = node.children[0].value;
            toc.push({ id, title: heading });
          }
        });
      };
    })
    .use(rehypeStringify)
    .process(fileContent);

  return toc;
}

Expected behavior

Types should be recognized.

Actual behavior

node is of type 'never'

Affected runtime and version

node@20 lts

Affected package manager and version

No response

Affected OS and version

No response

Build and bundle tools

No response

Is it safe to assume this is synchronous and will remain so? Might be helpful to add this in readme somewhere. 😄

e.g.

  var defs = []
  visit(ast, function (node, index, parent) {
    if (node.type === 'definition') {
      defs.push(node)
    }
  })
  console.log(defs)