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 5 commits into
base: main
Choose a base branch
from
Open
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
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
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] Does the team want to type check in CI separate from the build step? As in, was it intended that npm run build should be what type-checks?

Copy link
Member

Choose a reason for hiding this comment

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

npm run build is intended to do everything in preparation for a package being published (Including tsc, running additional scripts, running Rollup, etc.).

If we want to highlight type checking errors, then I think it's fine to have as a separate CI check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great. I've found it to be useful to have explicit checks for each of (lint, build, test, etc.) split out. What happens over time in larger repos is that type-checking time grows to be much much longer than build times, so folks reasonably want to be able to see results from tsc on its own separately from / in parallel to build. Both in CI and locally.


- 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/*/dist/`.

```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",
"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
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.

1 change: 0 additions & 1 deletion 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
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 don't get any type errors in my editor or from npm run build on main after removing this comment directive. Is there another command I'm missing?

Copy link
Member

Choose a reason for hiding this comment

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

Leftover comment from before I figured out how to get it to work. 😄

import levn from "levn";

//-----------------------------------------------------------------------------
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
11 changes: 11 additions & 0 deletions tsconfig.base.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"compilerOptions": {
"allowJs": true,
"checkJs": true,
"declaration": true,
"emitDeclarationOnly": true,
"module": "NodeNext",
"moduleResolution": "NodeNext",
"target": "ES2022"
}
}
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
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;
}