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

Merged
merged 15 commits into from
Nov 22, 2024
Merged
Show file tree
Hide file tree
Changes from 10 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
18 changes: 18 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,24 @@ on:
- main

jobs:
type_check:
name: Type Check
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4

- uses: actions/setup-node@v4
with:
node-version: "lts/*"

- name: Install Packages
run: |
npm install
npm run build
Copy link
Member

Choose a reason for hiding this comment

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

doubt: why are we running a build? aren't we type checking on source?

Copy link
Member

Choose a reason for hiding this comment

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

The types definition files aren't there until we build.

Copy link
Member

Choose a reason for hiding this comment

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

If that is so, doesn't package build step fail when the type is wrong for the package? 🤔 do we need another tsc command to run separately on the main repo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that is true. That's a pain I've just dealt with in other repos. In theory we could do a strategy like || 0 to ignore any failures.

Copy link
Member

Choose a reason for hiding this comment

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

just to re iterate, my doubt was why we'd need to run build command and test:types on source files?

  • the build process itself has a in-built tsc check? if the build fails then tsc is also expected to fail?
  • we cannot have a tsc check separately because its dependent on build (which in itself has a tsc check?)

now the question resolves to do we have any other ts files apart from the ts files that are built in the repo? if yes then its better to have a tsc for fallback

(sorry for the confusion 😅)

Copy link
Member

Choose a reason for hiding this comment

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

That's a good point. I hadn't noticed that test:types just runs tsc...for some reason I had it in my head that there was something else running there.

So yes, npm run build actually runs tsc, so we don't need something separate running 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.

Just clarifying - is there a request for changes?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, there's no need to run tsc separately because that's already happening via npm run build.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah got it. Thanks, I see now - sorry for giving you the runaround harish-sethuraman.


- name: Type Check Files
run: npm run test:types

verify_files:
name: Verify Files
runs-on: ubuntu-latest
Expand Down
40 changes: 40 additions & 0 deletions CONTRIBUTING.md
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
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.
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved

```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 test:types
```

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

```shell
npm run test:types -- --watch
```
4 changes: 3 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@
"lint:fix": "eslint --fix .",
"fmt": "prettier --write .",
"fmt:check": "prettier --check .",
"test:jsr": "npm run test:jsr --workspaces --if-present"
"test:jsr": "npm run test:jsr --workspaces --if-present",
"test:types": "tsc"
},
"workspaces": [
"packages/*"
Expand All @@ -27,6 +28,7 @@
"!(*.{js,ts})": "prettier --write --ignore-unknown"
},
"devDependencies": {
"@types/mocha": "^10.0.7",
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
"eslint": "^9.11.1",
"eslint-config-eslint": "^11.0.0",
"got": "^14.4.1",
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"
}
}
1 change: 1 addition & 0 deletions packages/plugin-kit/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
"license": "Apache-2.0",
"devDependencies": {
"@eslint/core": "^0.7.0",
"@types/levn": "^0.4.0",
"c8": "^9.1.0",
"mocha": "^10.4.0",
"rollup": "^4.16.2",
Expand Down
8 changes: 0 additions & 8 deletions packages/plugin-kit/src/@types/levn.d.ts

This file was deleted.

4 changes: 2 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,8 @@ 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)) || {};
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved

/*
* When the configuration has any invalid severities, it should be completely
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 @@ -73,8 +73,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
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
* 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
}
}