types: add export to package.json, test with strict mode and node16 resolution

[ChristianMurphy]

Done:

• [x] use node16 ESM resolution
• [x] add type export to package.json
• [x] test in strict mode
• [x] make sure types match up with implementation (currently failing in strict mode)

resolves https://github.com/Rich-Harris/estree-walker/issues/35 resolves https://github.com/Rich-Harris/estree-walker/issues/28 resolves https://github.com/Rich-Harris/estree-walker/pull/24

[wooorm]

@ChristianMurphy What’s blocking this?

[ChristianMurphy]

What’s blocking this?

Time, more specifically a lack thereof. This is mostly ready, that last remaining item here is unraveling some type inconsistencies in the visit method in sync.js and async.js which are a bit complex.

I think there are two separate things which need to be resolved in the visit method.

1. Resolve inconsistency on whether params are optional or not, externally they appear to be optional, internally they appear to be required.
2. Sort out some edge cases in the for (const key in node) { loop, where source location and comments potentially break assumptions in the code.

After those are resolved it would be nice to get https://github.com/Rich-Harris/estree-walker/pull/34#discussion_r1062455750 sorted, which will likely require an issue upstream at TypeScript, some back and forth, and some lead time waiting for a release.

I'll aim to keep chipping away at the remaining items. If you or others have time to pitch in, contributions are welcome! And would help speed the timeline up.

[wooorm]

Here’s a diff to make the types and tests work:

diff --git a/src/async.js b/src/async.js
index 64f6933..f068c71 100644
--- a/src/async.js
+++ b/src/async.js
@@ -6,9 +6,9 @@ import { WalkerBase } from './walker.js';
  * @typedef {(
  *    this: WalkerContext,
  *    node: Node,
- *    parent: Node,
- *    key: string | number | symbol,
- *    index: number
+ *    parent: Node | null,
+ *    key: string | number | symbol | null | undefined,
+ *    index: number | null | undefined
  * ) => Promise<void>} AsyncHandler
  */

@@ -47,9 +47,9 @@ export class AsyncWalker extends WalkerBase {
    /**
     * @template {Node} Parent
     * @param {Node} node
-    * @param {Parent} parent
-    * @param {keyof Parent} prop
-    * @param {number} index
+    * @param {Parent | null} parent
+    * @param {keyof Parent} [prop]
+    * @param {number | null} [index]
     * @returns {Promise<Node | null>}
     */
    async visit(node, parent, prop, index) {
@@ -84,22 +84,28 @@ export class AsyncWalker extends WalkerBase {
                if (removed) return null;
            }

-           for (const key in node) {
-               const value = node[/** @type {keyof Node} */ (key)];
-
-               if (typeof value !== "object") {
-                   continue;
-               } else if (Array.isArray(value)) {
-                   for (let i = 0; i < value.length; i += 1) {
-                       if (value[i] !== null && typeof value[i].type === 'string') {
-                           if (!(await this.visit(value[i], node, key, i))) {
-                               // removed
-                               i--;
+           /** @type {keyof Node} */
+           let key;
+
+           for (key in node) {
+               /** @type {unknown} */
+               const value = node[key];
+
+               if (value && typeof value === 'object') {
+                   if (Array.isArray(value)) {
+                       const nodes = /** @type {Array<unknown>} */ (value);
+                       for (let i = 0; i < nodes.length; i += 1) {
+                           const item = nodes[i];
+                           if (isNode(item)) {
+                               if (!(await this.visit(item, node, key, i))) {
+                                   // removed
+                                   i--;
+                               }
                            }
                        }
+                   } else if (isNode(value)) {
+                       await this.visit(value, node, key, null);
                    }
-               } else if (value !== null && typeof value.type === "string") {
-                   await this.visit(value, node, key, null);
                }
            }

@@ -132,3 +138,15 @@ export class AsyncWalker extends WalkerBase {
        return node;
    }
 }
+
+/**
+ * Ducktype a node.
+ *
+ * @param {unknown} value
+ * @returns {value is Node}
+ */
+function isNode(value) {
+   return (
+       value !== null && typeof value === 'object' && 'type' in value && typeof value.type === 'string'
+   );
+}
diff --git a/src/sync.js b/src/sync.js
index a7ad898..171fb36 100644
--- a/src/sync.js
+++ b/src/sync.js
@@ -6,9 +6,9 @@ import { WalkerBase } from './walker.js';
  * @typedef {(
  *    this: WalkerContext,
  *    node: Node,
- *    parent: Node,
- *    key?: string | number | symbol | undefined,
- *    index?: number | undefined
+ *    parent: Node | null,
+ *    key: string | number | symbol | null | undefined,
+ *    index: number | null | undefined
  * ) => void} SyncHandler
  */

@@ -47,9 +47,9 @@ export class SyncWalker extends WalkerBase {
    /**
     * @template {Node} Parent
     * @param {Node} node
-    * @param {Parent} parent
-    * @param {keyof Parent} prop
-    * @param {number} index
+    * @param {Parent | null} parent
+    * @param {keyof Parent} [prop]
+    * @param {number | null} [index]
     * @returns {Node | null}
     */
    visit(node, parent, prop, index) {
@@ -84,22 +84,28 @@ export class SyncWalker extends WalkerBase {
                if (removed) return null;
            }

-           for (const key in node) {
-               const value = node[/** @type {keyof Node} */(key)];
-
-               if (typeof value !== "object") {
-                   continue;
-               } else if (Array.isArray(value)) {
-                   for (let i = 0; i < value.length; i += 1) {
-                       if (value[i] !== null && typeof value[i].type === 'string') {
-                           if (!this.visit(value[i], node, key, i)) {
-                               // removed
-                               i--;
+           /** @type {keyof Node} */
+           let key;
+
+           for (key in node) {
+               /** @type {unknown} */
+               const value = node[key];
+
+               if (value && typeof value === 'object') {
+                   if (Array.isArray(value)) {
+                       const nodes = /** @type {Array<unknown>} */ (value);
+                       for (let i = 0; i < nodes.length; i += 1) {
+                           const item = nodes[i];
+                           if (isNode(item)) {
+                               if (!this.visit(item, node, key, i)) {
+                                   // removed
+                                   i--;
+                               }
                            }
                        }
+                   } else if (isNode(value)) {
+                       this.visit(value, node, key, null);
                    }
-               } else if (value !== null && typeof value.type === "string") {
-                   this.visit(value, node, key, null);
                }
            }

@@ -132,3 +138,15 @@ export class SyncWalker extends WalkerBase {
        return node;
    }
 }
+
+/**
+ * Ducktype a node.
+ *
+ * @param {unknown} value
+ * @returns {value is Node}
+ */
+function isNode(value) {
+   return (
+       value !== null && typeof value === 'object' && 'type' in value && typeof value.type === 'string'
+   );
+}
diff --git a/src/walker.js b/src/walker.js
index cfbc406..0c54bb9 100644
--- a/src/walker.js
+++ b/src/walker.js
@@ -28,31 +28,31 @@ export class WalkerBase {

    /**
     * @template {Node} Parent
-    * @param {Parent} parent
-    * @param {keyof Parent} prop
-    * @param {number | null} index
+    * @param {Parent | null | undefined} parent
+    * @param {keyof Parent | null | undefined} prop
+    * @param {number | null | undefined} index
     * @param {Node} node
     */
    replace(parent, prop, index, node) {
-       if (parent) {
+       if (parent && prop) {
            if (index !== null && typeof index !== 'undefined') {
-               /** @type {Array<Node>} */(parent[prop])[index] = node;
+               /** @type {Array<Node>} */ (parent[prop])[index] = node;
            } else {
-               /** @type {Node} */(parent[prop]) = node;
+               /** @type {Node} */ (parent[prop]) = node;
            }
        }
    }

    /**
     * @template {Node} Parent
-    * @param {Parent} parent
-    * @param {keyof Parent} prop
-    * @param {number} index
+    * @param {Parent | null | undefined} parent
+    * @param {keyof Parent | null | undefined} prop
+    * @param {number | null | undefined} index
     */
    remove(parent, prop, index) {
-       if (parent) {
-           if (index !== null) {
-               /** @type {Array<Node>} */(parent[prop]).splice(index, 1);
+       if (parent && prop) {
+           if (index !== null && index !== undefined) {
+               /** @type {Array<Node>} */ (parent[prop]).splice(index, 1);
            } else {
                delete parent[prop];
            }

[wooorm]

where source location and comments potentially break assumptions in the code.

This is an intentional decision I believe here: to allow arbitrary and even unknown nodes, while having less code, instead of tracking which nodes are valid. Otherwise you’d need a big list like https://github.com/eslint/eslint-visitor-keys/tree/main/lib.

[ChristianMurphy]

That did the trick, thanks @wooorm! Applied in https://github.com/Rich-Harris/estree-walker/pull/34/commits/52e383977bc06e0cc81d761a0c2fe0b27acca7e2

[ChristianMurphy]

@Rich-Harris this is ready for a review when you have a moment

[Rich-Harris]

released 3.0.3. thank you!

[ChristianMurphy]

Thanks @Rich-Harris! 🙇