-
Notifications
You must be signed in to change notification settings - Fork 63
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,3 +8,4 @@ npm-debug.log | |
yarn.lock | ||
package-lock.json | ||
pnpm-lock.yaml | ||
dist |
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" } | ||
] | ||
}) | ||
] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
}; |
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; |
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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
* | ||
* 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"); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
{ | ||
"extends": "./tsconfig.json", | ||
"files": ["dist/esm/index.js"] | ||
} |
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", | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Strict mode is a strong recommendation for TypeScript projects:
Suggested change
If the codebase/team isn't ready for it just yet, there should be a TODO for adding it in. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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" | ||||||||
} | ||||||||
} |
There was a problem hiding this comment.
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 fromjsdoc.configs["flat/recommended"]
tojsdoc.configs["flat/recommended-typescript-flavor"]
(or some similar addition of TypeScript). https://github.com/gajus/eslint-plugin-jsdoc/blob/2fbd47c3d2b23a41d87b979daa56a648270d3675/README.md#eslintrcThere was a problem hiding this comment.
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
.