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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
ae9c13c
chore: fully type check packages/*/src files
JoshuaKGoldberg Sep 3, 2024
cb49807
chore: remove unnecessary type assertion
JoshuaKGoldberg Sep 3, 2024
b0cbf31
chore: remove unnecessary unknown/object type changes
JoshuaKGoldberg Sep 3, 2024
ebeb4c8
Update CONTRIBUTING.md
JoshuaKGoldberg Sep 4, 2024
3e4c6e5
Remove strict mode
JoshuaKGoldberg Sep 5, 2024
cb403a2
Merge branch 'main' into tsc-on-src
JoshuaKGoldberg Sep 26, 2024
03bb82a
chore: use @types/levn
JoshuaKGoldberg Sep 26, 2024
47d20c8
chore: split out type_check job
JoshuaKGoldberg Sep 26, 2024
8631f45
switch to test:types
JoshuaKGoldberg Oct 19, 2024
6679eb6
Merge branch 'main'
JoshuaKGoldberg Oct 19, 2024
c50e7ca
Merge branch 'main'
JoshuaKGoldberg Nov 4, 2024
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
View file
Open in desktop
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


- name: Type Check Files
run: npm run tsc

verify_files:
name: Verify Files
runs-on: ubuntu-latest
Expand Down
40 changes: 40 additions & 0 deletions CONTRIBUTING.md
View file
Open in desktop
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
View file
Open in desktop
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.11.1",
"eslint-config-eslint": "^11.0.0",
Expand Down
10 changes: 2 additions & 8 deletions packages/compat/tsconfig.json
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
"license": "Apache-2.0",
"devDependencies": {
"@eslint/core": "^0.6.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
View file
Open in desktop

This file was deleted.

4 changes: 2 additions & 2 deletions packages/plugin-kit/src/config-comment-parser.js
View file
Open in desktop
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)) || {};
Copy link
Member

Choose a reason for hiding this comment

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

Should the parens enclose the entire expression?

Suggested change
/** @type {RulesConfig} */ (levn.parse("Object", string)) || {};
/** @type {RulesConfig} */ (levn.parse("Object", string) || {});

Copy link
Contributor Author

@JoshuaKGoldberg JoshuaKGoldberg Sep 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Great question: either works, but as-is is more precise.

From https://www.typescriptlang.org/docs/handbook/jsdoc-supported-types.html#casts, @type JSDoc annotations act as assertions1 on the parenthesized expression. Right now that expression includes only the levn.parse(...) call. Essentially it's (levn.parse(...) as RulesConfig) || {}.

I prefer that because, similar try/catch, IMO it's best to have assertions act on as small a range as possible. They're an inherently dangerous, use-at-your-own-risk feature.

The end result is the same here: RulesConfig | {} (note: that's the union type, not a || typo) collapses down to just RulesConfig in the type system.

Footnotes

  1. Normally in the TS docs called assertions (suggesting being purely in the type system) not casts (suggesting being a runtime operation). I was surprised to see it listed as a cast here... ↩

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanation. I guess I was reading this as "assign a value that is a RulesConfig or an empty object", meaning that the type of items isn't guaranteed to be RulesConfig. Maybe that's okay?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the types perspective, an empty object is RulesConfig. Record<string, RuleConfig> means roughly "all keys are string; all values are RuleConfig". If you have no keys, then great, that satisfies the shape.

Copy link
Member

Choose a reason for hiding this comment

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

Ah gotcha. Thanks for explaining 👍


/*
* When the configuration has any invalid severities, it should be completely
Expand Down
11 changes: 2 additions & 9 deletions packages/plugin-kit/tsconfig.json
View file
Open in desktop
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
View file
Open in desktop
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
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
View file
Open in desktop
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
View file
Open in desktop
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
}
}