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

ChristianMurphy · 2023-01-03T21:19:09Z

Done:

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

resolves #35
resolves #28
resolves #24

ChristianMurphy · 2023-01-04T20:56:39Z

src/async.js

+		this.should_skip = false;
+
+		/** @type {boolean} */
+		this.should_remove = false;
+
+		/** @type {Node | null} */
+		this.replacement = null;
+
+		/** @type {WalkerContext} */
+		this.context = {
+			skip: () => (this.should_skip = true),
+			remove: () => (this.should_remove = true),
+			replace: (node) => (this.replacement = node)
+		};


These shouldn't need to be here, they are defined by the super() calling WalkerBase.
But TypeScript + JSDoc seems to be unable to infer these from the extends WalkerBase on the class. 🤔

/cc @wooorm @remcohaszing have you ever run into this before? Do you know of any work arounds?

Apparently the culprit are these lines in visit:

const _should_skip = this.should_skip; const _should_remove = this.should_remove; const _replacement = this.replacement; // … this.should_skip = _should_skip; this.should_remove = _should_remove; this.replacement = _replacement;

It appears TypeScript is having problems recursively inferring these types from themselves. As a workaround, you can type-cast the values that are later assigned back. Note that this occurs twice!

const _should_skip = /** @type {boolean} */ (this.should_skip); const _should_remove = /** @type {boolean} */ (this.should_remove); const _replacement = /** @type {Node | null} */ (this.replacement);

This is definitely a bug in TypeScript. Here’s minimal reproduction. See what happens if you comment the line this.name = name.

class Foo { name = '' } class Bar extends Foo { rename() { const name = this.name this.name = name } }

Maybe you need to type fields? Such as like this: https://github.com/vfile/vfile/blob/03efac7cc3a67fe33bb7a0f1de29e17be322549e/lib/index.js#L122-L126

No, it’s currently done that way, but it doesn’t make a difference. It’s a bug with subclasses. Also it only happens in JS. If the exact same code in the minimal reproduction is added in a .ts file, TypeScript can infer the types perfectly fine.

remcohaszing · 2023-01-05T13:10:01Z

src/async.js

+			replace: (node) => (this.replacement = node)
+		};
+
+		/** @type {AsyncHandler | undefined} */


If you assign this.whatever = whatever in the constructor, TypeScript can infer its type, meaning this type annotation is redundant. There are multiple occurrences across multiple files. It’s not wrong, but personally I would omit them.

tsconfig.json

src/walker.js

package.json

remcohaszing · 2023-01-05T13:42:47Z

src/async.js

+		this.should_skip = false;
+
+		/** @type {boolean} */
+		this.should_remove = false;
+
+		/** @type {Node | null} */
+		this.replacement = null;
+
+		/** @type {WalkerContext} */
+		this.context = {
+			skip: () => (this.should_skip = true),
+			remove: () => (this.should_remove = true),
+			replace: (node) => (this.replacement = node)
+		};


No, it’s currently done that way, but it doesn’t make a difference. It’s a bug with subclasses. Also it only happens in JS. If the exact same code in the minimal reproduction is added in a .ts file, TypeScript can infer the types perfectly fine.

wooorm · 2023-01-17T10:13:36Z

@ChristianMurphy What’s blocking this?

ChristianMurphy · 2023-01-17T14:52:49Z

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.

Resolve inconsistency on whether params are optional or not, externally they appear to be optional, internally they appear to be required.
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 #34 (comment) 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.

Co-authored-by: Remco Haszing <remcohaszing@gmail.com>

wooorm · 2023-01-18T08:58:46Z

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 · 2023-01-18T09:03:02Z

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.

Co-authored-by: Titus Wormer <tituswormer@gmail.com>

ChristianMurphy · 2023-01-19T05:04:22Z

That did the trick, thanks @wooorm! Applied in 52e3839

Co-authored-by: Remco Haszing <remcohaszing@gmail.com>

ChristianMurphy · 2023-01-19T05:12:22Z

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

remcohaszing · 2023-01-19T09:17:48Z

src/sync.js

+	return (
+		value !== null && typeof value === 'object' && 'type' in value && typeof value.type === 'string'
+	);
+}


This function is defined twice. (It’s also in async.js.) I suggest to deduplicate this logic.

a lot of the code is duplicated tho, so, yeah it’s definitely a good idea to refactor, but as this PR is already doing a lot, I’d leave that for another PR!

Rich-Harris · 2023-01-20T02:27:20Z

released 3.0.3. thank you!

ChristianMurphy · 2023-01-20T03:59:09Z

Thanks @Rich-Harris! 🙇

ChristianMurphy mentioned this pull request Jan 3, 2023

types: add export to package.json, test with strict mode and node16 resolution Rich-Harris/periscopic#19

Merged

6 tasks

types: rework types for strict mode, run typescript in node16 esm mode

7cc986c

ChristianMurphy force-pushed the type/node-16-esm-types branch from 9c138cd to 7cc986c Compare January 4, 2023 20:53

ChristianMurphy commented Jan 4, 2023

View reviewed changes

remcohaszing reviewed Jan 5, 2023

View reviewed changes

ChristianMurphy mentioned this pull request Jan 16, 2023

Add module: node16 to tsconfig.json mdx-js/mdx#2228

Merged

ChristianMurphy and others added 2 commits January 17, 2023 07:57

remove unnecessary allowJs option from tsconfig.json

92dfef3

Co-authored-by: Remco Haszing <remcohaszing@gmail.com>

types: move estree types to dependencies, and upgrade to version 1

a964433

ChristianMurphy mentioned this pull request Jan 17, 2023

test: setup github actions, use node:test and node:assert/strict #36

Open

duck type the node type

52e3839

Co-authored-by: Titus Wormer <tituswormer@gmail.com>

ChristianMurphy changed the title ~~[WIP] types: add export to package.json, test with strict mode and node16 resolution~~ types: add export to package.json, test with strict mode and node16 resolution Jan 19, 2023

merge null and undefined test

a9dcf7f

Co-authored-by: Remco Haszing <remcohaszing@gmail.com>

wooorm approved these changes Jan 19, 2023

View reviewed changes

remcohaszing reviewed Jan 19, 2023

View reviewed changes

Rich-Harris approved these changes Jan 20, 2023

View reviewed changes

Rich-Harris merged commit 107ea0a into Rich-Harris:master Jan 20, 2023

ChristianMurphy deleted the type/node-16-esm-types branch January 20, 2023 03:30

jasonlyu123 mentioned this pull request Jun 2, 2023

fix build and test with svelte 4 sveltejs/language-tools#2041

Merged

bashmish mentioned this pull request Oct 21, 2024

fix(estree-walker): bump estree-walker to v3 rollup/plugins#1786

Draft

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ChristianMurphy commented Jan 3, 2023 •

edited

Loading

ChristianMurphy Jan 4, 2023

remcohaszing Jan 5, 2023

wooorm Jan 5, 2023

remcohaszing Jan 5, 2023

remcohaszing Jan 5, 2023

remcohaszing Jan 5, 2023

wooorm commented Jan 17, 2023

ChristianMurphy commented Jan 17, 2023 •

edited

Loading

wooorm commented Jan 18, 2023

wooorm commented Jan 18, 2023 •

edited

Loading

ChristianMurphy commented Jan 19, 2023

ChristianMurphy commented Jan 19, 2023

remcohaszing Jan 19, 2023

wooorm Jan 19, 2023 •

edited

Loading

Rich-Harris commented Jan 20, 2023

ChristianMurphy commented Jan 20, 2023

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

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

Conversation

ChristianMurphy commented Jan 3, 2023 • edited Loading

ChristianMurphy Jan 4, 2023

Choose a reason for hiding this comment

remcohaszing Jan 5, 2023

Choose a reason for hiding this comment

wooorm Jan 5, 2023

Choose a reason for hiding this comment

remcohaszing Jan 5, 2023

Choose a reason for hiding this comment

remcohaszing Jan 5, 2023

Choose a reason for hiding this comment

remcohaszing Jan 5, 2023

Choose a reason for hiding this comment

wooorm commented Jan 17, 2023

ChristianMurphy commented Jan 17, 2023 • edited Loading

wooorm commented Jan 18, 2023

wooorm commented Jan 18, 2023 • edited Loading

ChristianMurphy commented Jan 19, 2023

ChristianMurphy commented Jan 19, 2023

remcohaszing Jan 19, 2023

Choose a reason for hiding this comment

wooorm Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

Rich-Harris commented Jan 20, 2023

ChristianMurphy commented Jan 20, 2023

ChristianMurphy commented Jan 3, 2023 •

edited

Loading

ChristianMurphy commented Jan 17, 2023 •

edited

Loading

wooorm commented Jan 18, 2023 •

edited

Loading

wooorm Jan 19, 2023 •

edited

Loading