-
-
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?
Conversation
@@ -27,6 +27,9 @@ jobs: | |||
- name: Lint Files | |||
run: npm run lint | |||
|
|||
- name: Type Check Files | |||
run: npm run tsc |
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 (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.
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.
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
and README.md
. I think it's more important that the information exists somewhere rather than worrying about exactly where at this point.
@@ -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 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.
@@ -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 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?
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.
Leftover comment from before I figured out how to get it to work. 😄
* 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 comment
The reason will be displayed to describe this comment to others. Learn more.
[Fix] Typo: missing >
closing out Map
.
tsconfig.base.json
Outdated
// 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 comment
The reason will be displayed to describe this comment to others. Learn more.
[Explanation] strict
is generally a best practice, and has flags added to it over time. For anything that can't be added yet, the common practice is to disable them specifically with a TODO comment. Kind of like enabling a recommended preset in an ESLint config, with rules
overrides.
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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
👍 #118
types/levn.d.ts
Outdated
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 adding as much as I could here. I plan on sending to DefinitelyTyped soon. Did I get the types right? (I've never used levn
)
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.
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 tsc
happy and seemed to make sense based on the 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.
@@ -27,6 +27,9 @@ jobs: | |||
- name: Lint Files | |||
run: npm run lint | |||
|
|||
- name: Type Check Files | |||
run: npm run tsc |
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 (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.
### 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 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.
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.
Yes, it has a dependency on its own files right now:
$ npm run lint
> eslint-rewrite@1.0.0 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
> eslint-rewrite@1.0.0 lint
> eslint .
$ |
@@ -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 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. 😄
types/levn.d.ts
Outdated
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.
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 tsc
happy and seemed to make sense based on the docs.
Co-authored-by: Nicholas C. Zakas <nicholas@humanwhocodes.com>
Prerequisites checklist
What is the purpose of this pull request?
Creates a root-level
tsconfig.json
that can type check all project files, along with aci.yml
step to do so in CI.What changes did you make? (Give an overview)
This PR includes two common TypeScript practices for monorepos:
tsconfig.base.json
: to unify & share the common TSConfig settings used by all projectstsconfig.json
: so editors have that includes all filesThat root-level
tsconfig.json
is necessary for typed linting with the recommended typescript-eslint project service, as noted by @snitin315 and myself as a followup in #90 -> #90 (comment).Also fixes a few type errors here and there. I'm posting comments in the PR.
Notably, this PR does not set up project references. Doing so requires touching files on disk, which I'm not confident enough in this repo to do on my own unprompted. Instead, the root-level
tsconfig.json
hasnoEmit: true
so it's purely used for type checking.Related Issues
Followup to #90, which is a PR. Would the team like me to file more granular issues? I wasn't sure how much the team wants some or all of this change. My intent is to continue enabling strict TypeScript flags after this, unless directed otherwise.
Is there anything you'd like reviewers to focus on?
I'm applying the practices I see as common + good in TypeScript-land, but am not totally sure I interpreted the existing repo setup right. Very much seeking to understand. 🙂