-
-
Notifications
You must be signed in to change notification settings - Fork 12
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
base: main
Are you sure you want to change the base?
Changes from all commits
ae9c13c
cb49807
b0cbf31
ebeb4c8
3e4c6e5
cb403a2
03bb82a
47d20c8
8631f45
6679eb6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it has a dependency on its own files right now:
Running a build fixes that:
|
||
|
||
```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 | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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/*" | ||
|
@@ -27,6 +28,7 @@ | |
"!(*.{js,ts})": "prettier --write --ignore-unknown" | ||
}, | ||
"devDependencies": { | ||
"@types/mocha": "^10.0.7", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Explanation] This isn't strictly necessary with |
||
"eslint": "^9.11.1", | ||
"eslint-config-eslint": "^11.0.0", | ||
"got": "^14.4.1", | ||
|
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" | ||
} | ||
} |
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" | ||
} | ||
} |
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" | ||
} | ||
} |
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" | ||
} | ||
} |
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" | ||
} | ||
} |
This file was deleted.
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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"; | ||||||
|
||||||
//----------------------------------------------------------------------------- | ||||||
|
@@ -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)) || {}; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should the parens enclose the entire expression?
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, I prefer that because, similar The end result is the same here: Footnotes
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From the types perspective, an empty object is There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||||||
|
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 | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Fix] Typo: missing |
||
* dependencies between packages. | ||
* @returns {Array<string>} An array of directories to be built in order. | ||
*/ | ||
|
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" | ||
} | ||
} |
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 | ||
} | ||
} |
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.
[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?
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.
I think it's fine for now. I'm always torn between
CONTRIBUTING.md
andREADME.md
. I think it's more important that the information exists somewhere rather than worrying about exactly where at this point.