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

feat: Add type checking and type definitions #266

Merged
merged 3 commits into from
Aug 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,4 @@ npm-debug.log
yarn.lock
package-lock.json
pnpm-lock.yaml
dist
16 changes: 12 additions & 4 deletions package.json

Choose a reason for hiding this comment

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

(commenting here because eslint.config.js isn't changed) Now that files are being generally validated & type checked with TypeScript, the ESLint config should switch from jsdoc.configs["flat/recommended"] to jsdoc.configs["flat/recommended-typescript-flavor"] (or some similar addition of TypeScript). https://github.com/gajus/eslint-plugin-jsdoc/blob/2fbd47c3d2b23a41d87b979daa56a648270d3675/README.md#eslintrc

Copy link
Member Author

Choose a reason for hiding this comment

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

That change would need to be made in the main repo, which contains eslint-config-eslint.

Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,15 @@
"url": "https://github.com/btmills"
},
"type": "module",
"main": "src/index.js",
"main": "dist/esm/index.js",
"types": "dist/esm/index.d.ts",
"exports": {
"import": {
"default": "./src/index.js"
"default": "./dist/esm/index.js"
}
},
"files": [
"src"
"dist"
],
"publishConfig": {
"access": "public"
Expand All @@ -34,18 +35,25 @@
],
"scripts": {
"lint": "eslint .",
"lint:fix": "eslint --fix .",
"build:dedupe-types": "node tools/dedupe-types.js dist/esm/index.js",
"build": "rollup -c && npm run build:dedupe-types && tsc -p tsconfig.esm.json",
"prepare": "node ./npm-prepare.cjs",
"test": "c8 mocha \"tests/**/*.test.js\" --timeout 30000"
},
"devDependencies": {
"@eslint/core": "^0.2.0",
"@eslint/js": "^9.4.0",
"@types/eslint": "^9.6.0",
"c8": "^10.1.2",
"chai": "^5.1.1",
"eslint": "^9.4.0",
"eslint-config-eslint": "^11.0.0",
"globals": "^15.1.0",
"mocha": "^10.6.0"
"mocha": "^10.6.0",
"rollup": "^4.19.0",
"rollup-plugin-copy": "^3.5.0",
"typescript": "^5.5.4"
},
"dependencies": {
"mdast-util-from-markdown": "^2.0.1"
Expand Down
19 changes: 19 additions & 0 deletions rollup.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import copy from "rollup-plugin-copy";

export default {
input: "src/index.js",
output: [
{
file: "dist/esm/index.js",
format: "esm",
banner: '// @ts-self-types="./index.d.ts"'
}
],
plugins: [
copy({
targets: [
{ src: "src/types.ts", dest: "dist/esm" }
]
})
]

Choose a reason for hiding this comment

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

Is using Rollup + plugins a settled choice? I've had an easier time with tsup as a more TypeScript-native builder.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's what we've been using in all of the type-checked repos so far. Given that it's working for us, I don't think this PR is the place to ponder alternatives. However, it is a bit clunky, so an exploration of another option would be welcome as a separate PR.

};
9 changes: 9 additions & 0 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,18 @@

import { processor } from "./processor.js";

//-----------------------------------------------------------------------------
// Type Definitions
//-----------------------------------------------------------------------------

/** @typedef {import("eslint").Linter.RulesRecord} RulesRecord*/
/** @typedef {import("eslint").ESLint.Plugin} Plugin */

//-----------------------------------------------------------------------------
// Exports
//-----------------------------------------------------------------------------

/** @type {RulesRecord} */
const rulesConfig = {

// The Markdown parser automatically trims trailing
Expand All @@ -36,6 +44,7 @@ const rulesConfig = {
"unicode-bom": "off"
};

/** @type {Plugin} */
const plugin = {
meta: {
name: "@eslint/markdown",
Expand Down
76 changes: 45 additions & 31 deletions src/processor.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,30 +3,30 @@
* @author Brandon Mills
*/

/**
* @typedef {import('eslint/lib/shared/types').LintMessage} Message
* @typedef {Object} ASTNode
* @property {string} type The type of node.
* @property {string} [lang] The language that the node is in
* @typedef {Object} RangeMap
* @property {number} indent Number of code block indent characters trimmed from
* the beginning of the line during extraction.
* @property {number} js Offset from the start of the code block's range in the
* extracted JS.
* @property {number} md Offset from the start of the code block's range in the
* original Markdown.
* @typedef {Object} BlockBase
* @property {string} baseIndentText Leading whitespace text for the block.
* @property {string[]} comments Comments inside of the JavaScript code.
* @property {RangeMap[]} rangeMap A list of offset-based adjustments, where
* lookups are done based on the `js` key, which represents the range in the
* linted JS, and the `md` key is the offset delta that, when added to the JS
* range, returns the corresponding location in the original Markdown source.
* @typedef {ASTNode & BlockBase} Block
*/
//-----------------------------------------------------------------------------
// Imports
//-----------------------------------------------------------------------------

import { fromMarkdown } from "mdast-util-from-markdown";

//-----------------------------------------------------------------------------
// Type Definitions
//-----------------------------------------------------------------------------

/** @typedef {import("./types.ts").Block} Block */
/** @typedef {import("./types.ts").RangeMap} RangeMap */
/** @typedef {import("mdast").Node} Node */
/** @typedef {import("mdast").Parent} ParentNode */
/** @typedef {import("mdast").Code} CodeNode */
/** @typedef {import("mdast").Html} HtmlNode */
/** @typedef {import("eslint").Linter.LintMessage} Message */
/** @typedef {import("eslint").Rule.Fix} Fix */
/** @typedef {import("eslint").AST.Range} Range */

//-----------------------------------------------------------------------------
// Helpers
//-----------------------------------------------------------------------------

const UNSATISFIABLE_RULES = new Set([
"eol-last", // The Markdown parser strips trailing newlines in code fences
"unicode-bom" // Code blocks will begin in the middle of Markdown files
Expand All @@ -40,8 +40,8 @@ const blocksCache = new Map();

/**
* Performs a depth-first traversal of the Markdown AST.
* @param {ASTNode} node A Markdown AST node.
* @param {{[key: string]: (node: ASTNode) => void}} callbacks A map of node types to callbacks.
* @param {Node} node A Markdown AST node.
* @param {{[key: string]: (node?: Node) => void}} callbacks A map of node types to callbacks.
* @returns {void}
*/
function traverse(node, callbacks) {
Expand All @@ -51,9 +51,11 @@ function traverse(node, callbacks) {
callbacks["*"]();
}

if (typeof node.children !== "undefined") {
for (let i = 0; i < node.children.length; i++) {
traverse(node.children[i], callbacks);
const parent = /** @type {ParentNode} */ (node);

if (typeof parent.children !== "undefined") {
for (let i = 0; i < parent.children.length; i++) {
traverse(parent.children[i], callbacks);
nzakas marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
Expand Down Expand Up @@ -92,7 +94,7 @@ const leadingWhitespaceRegex = /^[>\s]*/u;
/**
* Gets the offset for the first column of the node's first line in the
* original source text.
* @param {ASTNode} node A Markdown code block AST node.
* @param {Node} node A Markdown code block AST node.
* @returns {number} The offset for the first column of the node's first line.
*/
function getBeginningOfLineOffset(node) {
Expand All @@ -103,7 +105,7 @@ function getBeginningOfLineOffset(node) {
* Gets the leading text, typically whitespace with possible blockquote chars,
* used to indent a code block.
* @param {string} text The text of the file.
* @param {ASTNode} node A Markdown code block AST node.
* @param {Node} node A Markdown code block AST node.
* @returns {string} The text from the start of the first line to the opening
* fence of the code block.
*/
Expand Down Expand Up @@ -137,7 +139,7 @@ function getIndentText(text, node) {
* differences within the line, so the mapping need only provide the offset
* delta at the beginning of each line.
* @param {string} text The text of the file.
* @param {ASTNode} node A Markdown code block AST node.
* @param {Node} node A Markdown code block AST node.
* @param {string[]} comments List of configuration comment strings that will be
* inserted at the beginning of the code block.
* @returns {RangeMap[]} A list of offset-based adjustments, where lookups are
Expand Down Expand Up @@ -265,6 +267,12 @@ function preprocess(text, filename) {
"*"() {
htmlComments = [];
},

/**
* Visit a code node.
* @param {CodeNode} node The visited node.
* @returns {void}
*/
code(node) {
if (node.lang) {
const comments = [];
Expand All @@ -288,6 +296,12 @@ function preprocess(text, filename) {
});
}
},

/**
* Visit an HTML node.
* @param {HtmlNode} node The visited node.
* @returns {void}
*/
html(node) {
const comment = getComment(node.value);

Expand Down Expand Up @@ -357,7 +371,7 @@ function adjustBlock(block) {

if (message.fix) {
adjustedFix.fix = {
range: message.fix.range.map(range => {
range: /** @type {Range} */ (message.fix.range.map(range => {

// Advance through the block's range map to find the last
// matching range by finding the first range too far and
Expand All @@ -370,7 +384,7 @@ function adjustBlock(block) {

// Apply the mapping delta for this range.
return range + block.rangeMap[i - 1].md;
}),
})),
text: message.fix.text.replace(/\n/gu, `\n${block.baseIndentText}`)
};
}
Expand Down
19 changes: 19 additions & 0 deletions src/types.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import type { Node } from "mdast";
import type { Linter } from "eslint";


export interface RangeMap {
indent: number;
js: number;
md: number;
}

export interface BlockBase {
baseIndentText: string;
comments: string[];
rangeMap: RangeMap[];
}

export interface Block extends Node, BlockBase {}

export type Message = Linter.LintMessage;
43 changes: 43 additions & 0 deletions tools/dedupe-types.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/**
* @fileoverview Strips typedef aliases from the rolled-up file. This
* is necessary because the TypeScript compiler throws an error when
* it encounters a duplicate typedef.

Choose a reason for hiding this comment

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

Do you have a reference here? My instinct is that one of the following must be true:

  • This is a case of the repo's tooling "holding it wrong": which would be a good thing to fix (e.g. by switching from Rollup to something like tsup that's more commonly used with TS builds)
  • This is an actual bug in TypeScript that we can link to & I'd want to know more about
  • This is a really common thing we'd want to have standardized tooling for (which would surprise me, but you never know!)

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought this was intended behavior? Basically, if you have two JSDoc typedefs with the same name, tsc errors out. You can test this for yourself by checking out this branch and removing the dedupe-types step from npm run build and then running it.

Copy link

@JoshuaKGoldberg JoshuaKGoldberg Jul 30, 2024

Choose a reason for hiding this comment

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

It might be intended behavior on the TypeScript side to error on this case, but it's not intended behavior for the case to be present in the first place. In other words: it's not intended that everybody writing types-in-JS should have to write tools to work around quirks of type checking and transpiling in this way. This kind of bespoke tooling shouldn't be normal or required for any kind of TypeScript projects.

Copy link
Member Author

Choose a reason for hiding this comment

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

Gotcha. I guess I assumed it was akin to defining the same variable twice with let or const.

*
* Usage:
* node scripts/strip-typedefs.js filename1.js filename2.js ...
*
* @author Nicholas C. Zakas
*/

//-----------------------------------------------------------------------------
// Imports
//-----------------------------------------------------------------------------

import fs from "node:fs";

//-----------------------------------------------------------------------------
// Main
//-----------------------------------------------------------------------------

// read files from the command line
const files = process.argv.slice(2);

files.forEach(filePath => {
const lines = fs.readFileSync(filePath, "utf8").split(/\r?\n/gu);
const typedefs = new Set();

const remainingLines = lines.filter(line => {
if (!line.startsWith("/** @typedef {import")) {
return true;
}

if (typedefs.has(line)) {
return false;
}

typedefs.add(line);
return true;
});

fs.writeFileSync(filePath, remainingLines.join("\n"), "utf8");
});
4 changes: 4 additions & 0 deletions tsconfig.esm.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"extends": "./tsconfig.json",
"files": ["dist/esm/index.js"]
}
14 changes: 14 additions & 0 deletions tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
{
"files": ["src/index.js"],
"compilerOptions": {
"declaration": true,
"emitDeclarationOnly": true,
"allowImportingTsExtensions": true,
"allowJs": true,
"checkJs": true,
"outDir": "dist/esm",
"target": "ES2022",

Choose a reason for hiding this comment

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

Strict mode is a strong recommendation for TypeScript projects:

Suggested change
"target": "ES2022",
"strict": true,
"target": "ES2022",

If the codebase/team isn't ready for it just yet, there should be a TODO for adding it in.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is an open question. It might be recommended, but I've found it to be overly cumbersome and I'm not sold that the value is worth the pain. We can follow up on that question later.

"moduleResolution": "NodeNext",
"module": "NodeNext"
}
}