-
-
Notifications
You must be signed in to change notification settings - Fork 7
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: validate type comments and generate .d.ts
#204
base: main
Are you sure you want to change the base?
Conversation
That's a good idea! |
Following up from this comment: eslint-community/eslint-plugin-n#169 (comment) I made a couple of utility changes to the .d.ts file I stole from your description 😄
|
I'll get around to update this PR real soon. One challenge right now is that this module support quite old versions of Node.js. I would prefer to align with That's a separate issue though, but one I would want to deal with before spending work on backporting changes to those older versions. cc @JoshuaKGoldberg, would be lovely with some |
Co-authored-by: Josh Goldberg ✨ <[email protected]>
rules: { | ||
semi: ["error", "never"], | ||
"semi-spacing": ["error", { before: false, after: true }], |
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.
There's something deeply funny to me about an eslint-community
project using ESLint for formatting.
"coverage": "opener ./coverage/lcov-report/index.html", | ||
"docs:build": "vitepress build docs", | ||
"docs:watch": "vitepress dev docs", | ||
"format": "npm run -s format:prettier -- --write", | ||
"format:prettier": "prettier .", | ||
"format:check": "npm run -s format:prettier -- --check", | ||
"lint": "eslint .", | ||
"lint:eslint": "eslint .", | ||
"lint:tsc": "tsc", |
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.
Naming: I wouldn't call TypeScript a "lint" task. Even if it acts essentially as a linter in this setup, I've found it confusing for newer contributors to see it named as one.
const operations = Object.freeze({ | ||
ArrayExpression(node, initialScope) { | ||
if (node.type !== "ArrayExpression") { | ||
return null | ||
} |
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.
🤔 are these if
s actually necessary, or just to appease a type checker that doesn't understand what's happening?
If it's the latter, the general right thing to do would be to fix TypeScript's understanding of the types so it doesn't yell at you.
For posterity: I tried adding /** @param {import("estree").ArrayExpression} node */
to the ArrayExpression
one and got a type error on operations
:
Type 'Readonly<{ ArrayExpression(node: ArrayExpression, initialScope: Scope | undefined): { value: any[]; } | null; AssignmentExpression(node: Node, initialScope: Scope | undefined): StaticValue | null; ... 14 more ...; UnaryExpression(node: Node, initialScope: Scope | undefined): { ...; } | ... 3 more ... | null; }>' is not assignable to type 'Readonly<Partial<Record<"CatchClause" | "ClassBody" | "Identifier" | "Literal" | "MethodDefinition" | "PrivateIdentifier" | "Program" | "Property" | "PropertyDefinition" | "SpreadElement" | ... 60 more ... | "VariableDeclaration", VisitorCallback>>>'.
Types of property 'ArrayExpression' are incompatible.
Type '(node: ArrayExpression, initialScope: Scope | undefined) => { value: any[]; } | null' is not assignable to type 'VisitorCallback'.ts(2322)
Tricky.
I got around it by:
- Renaming
types.mjs
totypes.d.ts
, so I could use "real" TypeScript syntax and not the annoying JSDoc sludge - Making the
VisitorCallback
type generic:
export type VisitorCallback<InNode extends Node> = (
node: InNode,
initialScope: eslint.Scope.Scope | undefined,
) => StaticValue | null
- Using a type annotation on the
ArrayExpression
member:
/** @type {Readonly<Partial<Record<import('eslint').Rule.NodeTypes, import("./types.js").VisitorCallback<any>>>>} */
const operations = Object.freeze({
/** @type {import("./types.js").VisitorCallback<import("estree").ArrayExpression>} */
ArrayExpression(node, initialScope) {
YMMV.
@@ -74,10 +72,11 @@ function replaceS(matcher, str, replacement) { | |||
* Replace a given string by a given matcher. | |||
* @param {PatternMatcher} matcher The pattern matcher. | |||
* @param {string} str The string to be replaced. | |||
* @param {(...strs[])=>string} replace The function to replace each matched part. | |||
* @param {(...strs: (string|number)[])=>string} replace The function to replace each matched part. |
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.
How far is this from being merged? Would it be possible to cherry pick this change explicitly?
Context:
I'm introducing an Eslint plugin to our workflow that is relying on this package. Our typechecking currently depends on maxNodeModuleJsDepth
since we have a monorepo including old js-with-types-in-jsdoc modules, resulting in this package being blocked in CI because of invalid syntax.
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.
As a person doing lots of js-with-types and have used maxNodeModuleJsDepth
, I would say: There's never an excuse to use maxNodeModuleJsDepth
anymore, since one can now compile type declarations from JSDoc declarations.
In a monorepo – either have TypeScript handle the entire monorepo as a single entity (thus allowing JSDoc in all parts) and handle individual module paths through paths
in tsconfig.json
or use something like @monorepo-utils/workspaces-to-typescript-project-references
to set up project references to compile type declarations for the different parts (haven't tried this specific approach with JS)
Published JSDoc comments are not meant for consumption by TypeScript – only published type declarations are
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.
How far is this from being merged?
On this topic, see #232 :)
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.
@voxpelli Thank you for the reply, I agree with this completely
Published JSDoc comments are not meant for consumption by TypeScript – only published type declarations are
We've not figured out how to adopt a better typescript config yet but some blockers were resolved in TS 5.5 so maybe we should give it another try, thank you for the pointers!
I'll be patching this on our end for now.
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.
@GNRSN Feel free to pick my brain on the types-in-js approach over at https://github.com/voxpelli/types-in-js :)
Created #234 to move this along, and #235 as a companion to that |
This is an alternative to #60 which also fixes #150
As this PR doesn't rewrite to TS its easier to keep up to date as other changes happen.
No files has been renamed, the build script remains largely the same, the code as well mostly.
The generated type file