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

chore: fully type check packages/*/src files #117

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
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
3 changes: 3 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ jobs:
- name: Lint Files
run: npm run lint

- name: Type Check Files
run: npm run tsc
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved

- name: Check Formatting
run: npm run fmt:check

Expand Down
40 changes: 40 additions & 0 deletions CONTRIBUTING.md
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[Question] I ended up writing this because I wanted to add a mention of how to type-check, and didn't know where to put it... is this the right place for rewrite contributing docs?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine for now. I'm always torn between CONTRIBUTING.md and README.md. I think it's more important that the information exists somewhere rather than worrying about exactly where at this point.

Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
# Contributing

Please be sure to read the contribution guidelines before making or requesting a change.

## Code of Conduct

This project adheres to the [OpenJS Foundation Code of Conduct](https://eslint.org/conduct). We kindly request that you read over our code of conduct before contributing.

## Commands

### Building

[Rollup](https://rollupjs.org) and [TypeScript](https://www.typescriptlang.org) are used to turn source files in `packages/*/src/` into outputs in `packages/*/lib/`.
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved

```shell
npm run build
```

### Linting

ESLint is linted using ESLint.
[Building](#building) the project must be done before it can lint itself.
Copy link
Member

Choose a reason for hiding this comment

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

Did you find this to be true? Linting generally happens on source files, so not sure why building would be required first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it has a dependency on its own files right now:

$ npm run lint

> [email protected] lint
> eslint .


Oops! Something went wrong! :(

ESLint: 9.9.1

Error: Cannot find module '/Users/josh/repos/rewrite/node_modules/@eslint/config-array/dist/cjs/index.cjs'
    at createEsmNotFoundErr (node:internal/modules/cjs/loader:1256:15)
    at finalizeEsmResolution (node:internal/modules/cjs/loader:1244:15)
    at resolveExports (node:internal/modules/cjs/loader:628:14)
    at Module._findPath (node:internal/modules/cjs/loader:718:31)
    at Module._resolveFilename (node:internal/modules/cjs/loader:1205:27)
    at Module._load (node:internal/modules/cjs/loader:1045:27)
    at TracingChannel.traceSync (node:diagnostics_channel:315:14)
    at wrapModuleLoad (node:internal/modules/cjs/loader:215:24)
    at Module.require (node:internal/modules/cjs/loader:1304:12)
    at require (node:internal/modules/helpers:123:16)

Running a build fixes that:

$ npm run build

# ... build output

 $ npm run lint

> [email protected] lint
> eslint .

$ |


```shell
npm run lint
```

### Type Checking

This project is written in JavaScript and uses [TypeScript](https://www.typescriptlang.org) to validate types declared in JSDoc comments.

```shell
npm run tsc
```

Add `--watch` to run in a "watch" mode:

```shell
npm run tsc -- --watch
```
2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
"build": "node scripts/build.js",
"build:readme": "node tools/update-readme.js",
"lint": "eslint .",
"tsc": "tsc",
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
"lint:fix": "eslint --fix .",
"fmt": "prettier --write .",
"fmt:check": "prettier --check .",
Expand All @@ -27,6 +28,7 @@
"!(*.{js,ts})": "prettier --write --ignore-unknown"
},
"devDependencies": {
"@types/mocha": "^10.0.7",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[Explanation] This isn't strictly necessary with "noImplicitAny": false. But it's just good to have types for dependencies in general.

"common-tags": "^1.8.2",
"eslint": "^9.4.0",
"eslint-config-eslint": "^11.0.0",
Expand Down
2 changes: 2 additions & 0 deletions packages/compat/src/fixup-rules.js
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ export function fixupRule(ruleDefinition) {
currentNode =
args[methodName === "onCodePathSegmentLoop" ? 2 : 1];

// @ts-expect-error -- method.call is any from Object.entries, but we know its type is fine
return method.call(visitor, ...args);
};

Expand All @@ -171,6 +172,7 @@ export function fixupRule(ruleDefinition) {
visitor[methodName] = (...args) => {
currentNode = args[0];

// @ts-expect-error -- method.call is any from Object.entries, but we know its type is fine
return method.call(visitor, ...args);
};
}
Expand Down
10 changes: 2 additions & 8 deletions packages/compat/tsconfig.json
Original file line number Diff line number Diff line change
@@ -1,13 +1,7 @@
{
"extends": "../../tsconfig.base.json",
"files": ["src/index.js"],
"compilerOptions": {
"declaration": true,
"emitDeclarationOnly": true,
"allowJs": true,
"checkJs": true,
"outDir": "dist/esm",
"target": "ES2022",
"moduleResolution": "NodeNext",
"module": "NodeNext"
"outDir": "dist/esm"
}
}
10 changes: 2 additions & 8 deletions packages/config-array/tsconfig.json
Original file line number Diff line number Diff line change
@@ -1,13 +1,7 @@
{
"extends": "../../tsconfig.base.json",
"files": ["src/index.js"],
"compilerOptions": {
"declaration": true,
"emitDeclarationOnly": true,
"allowJs": true,
"checkJs": true,
"outDir": "dist/esm",
"target": "ES2022",
"moduleResolution": "NodeNext",
"module": "NodeNext"
"outDir": "dist/esm"
}
}
10 changes: 2 additions & 8 deletions packages/core/tsconfig.json
Original file line number Diff line number Diff line change
@@ -1,13 +1,7 @@
{
"extends": "../../tsconfig.base.json",
"files": ["src/types.ts"],
"compilerOptions": {
"declaration": true,
"emitDeclarationOnly": true,
"allowJs": true,
"checkJs": true,
"outDir": "dist/esm",
"target": "ES2022",
"moduleResolution": "NodeNext",
"module": "NodeNext"
"outDir": "dist/esm"
}
}
10 changes: 2 additions & 8 deletions packages/migrate-config/tsconfig.json
Original file line number Diff line number Diff line change
@@ -1,13 +1,7 @@
{
"extends": "../../tsconfig.base.json",
"files": ["src/migrate-config-cli.js"],
"compilerOptions": {
"declaration": true,
"emitDeclarationOnly": true,
"allowJs": true,
"checkJs": true,
"outDir": "dist/esm",
"target": "ES2022",
"moduleResolution": "NodeNext",
"module": "NodeNext"
"outDir": "dist/esm"
}
}
10 changes: 2 additions & 8 deletions packages/object-schema/tsconfig.json
Original file line number Diff line number Diff line change
@@ -1,13 +1,7 @@
{
"extends": "../../tsconfig.base.json",
"files": ["src/index.js"],
"compilerOptions": {
"declaration": true,
"emitDeclarationOnly": true,
"allowJs": true,
"checkJs": true,
"outDir": "dist/esm",
"target": "ES2022",
"moduleResolution": "NodeNext",
"module": "NodeNext"
"outDir": "dist/esm"
}
}
8 changes: 0 additions & 8 deletions packages/plugin-kit/src/@types/levn.d.ts

This file was deleted.

5 changes: 3 additions & 2 deletions packages/plugin-kit/src/config-comment-parser.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
// Imports
//------------------------------------------------------------------------------

// @ts-ignore -- don't feel like fighting with TypeScript right now
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
import levn from "levn";

//-----------------------------------------------------------------------------
Expand Down Expand Up @@ -123,7 +122,9 @@ export class ConfigCommentParser {
parseJSONLikeConfig(string) {
// Parses a JSON-like comment by the same way as parsing CLI option.
try {
const items = levn.parse("Object", string) || {};
const items = /** @type {RulesConfig} */ (
levn.parse("Object", string) || {}
);

/*
* When the configuration has any invalid severities, it should be completely
Expand Down
4 changes: 2 additions & 2 deletions packages/plugin-kit/src/source-code.js
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ export class Directive {

/**
* The node representing the directive.
* @type {unknown}
* @type {object}
* @readonly
*/
node;
Expand All @@ -198,7 +198,7 @@ export class Directive {
* Creates a new instance.
* @param {Object} options The options for the directive.
* @param {"disable"|"enable"|"disable-next-line"|"disable-line"} options.type The type of directive.
* @param {unknown} options.node The node representing the directive.
* @param {object} options.node The node representing the directive.
* @param {string} options.value The value of the directive.
* @param {string} options.justification The justification for the directive.
*/
Expand Down
11 changes: 2 additions & 9 deletions packages/plugin-kit/tsconfig.json
Original file line number Diff line number Diff line change
@@ -1,15 +1,8 @@
{
"extends": "../../tsconfig.base.json",
"files": ["src/index.js"],
"compilerOptions": {
"declaration": true,
"emitDeclarationOnly": true,
"allowJs": true,
"checkJs": true,
"outDir": "dist/esm",
"target": "ES2022",
"moduleResolution": "NodeNext",
"module": "NodeNext",
"strict": true,
"typeRoots": ["node_modules/@types", "src/@types"]
"strict": true
}
}
4 changes: 2 additions & 2 deletions scripts/build.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,8 @@ async function calculatePackageDependencies(packageDirs) {
}

/**
* Creates an array of directories to be built in order to sastify dependencies.
* @param {Map<string,{name:string,dir:string,dependencies:Set<string>}} dependencies The
* Creates an array of directories to be built in order to satisfy dependencies.
* @param {Map<string,{name:string,dir:string,dependencies:Set<string>}>} dependencies The
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[Fix] Typo: missing > closing out Map.

* dependencies between packages.
* @returns {Array<string>} An array of directories to be built in order.
*/
Expand Down
17 changes: 17 additions & 0 deletions tsconfig.base.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
{
"compilerOptions": {
"allowJs": true,
"checkJs": true,
"declaration": true,
"emitDeclarationOnly": true,
"module": "NodeNext",
"moduleResolution": "NodeNext",
"strict": true,
"target": "ES2022",

// TODO: Over time, enable these strict options
"noImplicitAny": false,
"strictNullChecks": false,
"useUnknownInCatchVariables": false
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
}
}
8 changes: 8 additions & 0 deletions tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"extends": "./tsconfig.base.json",
"include": ["packages/*/src"],
"compilerOptions": {
"emitDeclarationOnly": false,
"noEmit": true
}
}
36 changes: 36 additions & 0 deletions types/levn.d.ts
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/**
* @todo This should be contributed up to DefinitelyTyped to make a `@types/levn` package.
*/
declare module "levn" {
export interface ParseOptions {
explicit?: boolean | undefined;
customTypes?: Record<string, CustomParseType>;
}

export interface CustomParseType<Value = unknown> {
typeOf: string;
validate: (value: unknown) => boolean;
cast: (value: unknown) => CastedValue<Value>;
}

export interface CastedValue<Value = unknown> {
type: string;
value: Value;
}

// TODO: This comes from type-check.
export type ParsedType = unknown;

export function parse(
type: string,
string: string,
options?: unknown,
): unknown;

export function parsedTypeParse(
parsedType: string,
options?: unknown,
): unknown;

export const VERSION: string;
}