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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 5 commits into from
Jan 20, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7cc986c
types: rework types for strict mode, run typescript in node16 esm mode
ChristianMurphy Jan 4, 2023
92dfef3
remove unnecessary allowJs option from tsconfig.json
ChristianMurphy Jan 17, 2023
a964433
types: move estree types to dependencies, and upgrade to version 1
ChristianMurphy Jan 17, 2023
52e3839
duck type the node type
ChristianMurphy Jan 19, 2023
a9dcf7f
merge null and undefined test
ChristianMurphy Jan 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
149 changes: 36 additions & 113 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 4 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,11 @@
"prepublishOnly": "tsc && npm test",
"test": "uvu test"
},
"dependencies": {
"@types/estree": "^1.0.0"
},
"devDependencies": {
"@types/estree": "0.0.42",
"typescript": "^3.7.5",
"typescript": "^4.9.0",
"uvu": "^0.5.1"
},
"files": [
Expand Down
96 changes: 65 additions & 31 deletions src/async.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,40 +1,56 @@
// @ts-check
import { WalkerBase } from './walker.js';

/** @typedef { import('estree').BaseNode} BaseNode */
/** @typedef { import('./walker').WalkerContext} WalkerContext */

/** @typedef {(
/**
* @typedef { import('estree').Node} Node
* @typedef { import('./walker.js').WalkerContext} WalkerContext
* @typedef {(
* this: WalkerContext,
* node: BaseNode,
* parent: BaseNode,
* key: string,
* index: number
* ) => Promise<void>} AsyncHandler */
* node: Node,
* parent: Node | null,
* key: string | number | symbol | null | undefined,
* index: number | null | undefined
* ) => Promise<void>} AsyncHandler
*/

export class AsyncWalker extends WalkerBase {
/**
*
* @param {AsyncHandler} enter
* @param {AsyncHandler} leave
* @param {AsyncHandler} [enter]
* @param {AsyncHandler} [leave]
*/
constructor(enter, leave) {
super();

/** @type {AsyncHandler} */
/** @type {boolean} */
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)
};
Comment on lines +25 to +38
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/** @type {AsyncHandler | undefined} */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

this.enter = enter;

/** @type {AsyncHandler} */
/** @type {AsyncHandler | undefined} */
this.leave = leave;
}

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

for (const key in node) {
/** @type {keyof Node} */
let key;

for (key in node) {
/** @type {unknown} */
const value = 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--;
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);
}
}

Expand Down Expand Up @@ -116,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'
);
}
Loading