-
-
Notifications
You must be signed in to change notification settings - Fork 10
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 4 commits
ae9c13c
cb49807
b0cbf31
ebeb4c8
3e4c6e5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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. [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 commentThe reason will be displayed to describe this comment to others. Learn more. I think it's fine for now. I'm always torn between |
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 tsc | ||
``` | ||
|
||
Add `--watch` to run in a "watch" mode: | ||
|
||
```shell | ||
npm run tsc -- --watch | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 .", | ||
|
@@ -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 |
||
"common-tags": "^1.8.2", | ||
"eslint": "^9.4.0", | ||
"eslint-config-eslint": "^11.0.0", | ||
|
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 | ||
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. [Question] I don't get any type errors in my editor or from 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. Leftover comment from before I figured out how to get it to work. 😄 |
||
import levn from "levn"; | ||
|
||
//----------------------------------------------------------------------------- | ||
|
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 |
---|---|---|
|
@@ -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 | ||
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,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 | ||
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] I know there was mention of not wanting some or all of these - is here a good place to discuss? 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. I think strict vs. not should be opened up as a separate discussion. 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. 👍 #118 |
||
} | ||
} |
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 commentThe reason will be displayed to describe this comment to others. Learn more. [Question] I ended up adding as much as I could here. I plan on sending to DefinitelyTyped soon. Did I get the types right? (I've never used 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. If you're planning on contributing to DefinitelyTyped, I'd rather wait for that than create a new top-level directory. Also, I'm not familiar enough with levn to really know if the types make sense. I just hacked something together that made 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. |
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; | ||
} |
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] 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?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.
npm run build
is intended to do everything in preparation for a package being published (Includingtsc
, 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.
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.
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.