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

JoshuaKGoldberg · 2024-09-03T21:39:24Z

Prerequisites checklist

I have read the contributing guidelines.

What is the purpose of this pull request?

Creates a root-level tsconfig.json that can type check all project files, along with a ci.yml step to do so in CI.

What changes did you make? (Give an overview)

This PR includes two common TypeScript practices for monorepos:

A tsconfig.base.json: to unify & share the common TSConfig settings used by all projects
A root-level tsconfig.json: so editors have that includes all files

That 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 has noEmit: 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. 🙂

.github/workflows/ci.yml

JoshuaKGoldberg · 2024-09-03T21:41:07Z

CONTRIBUTING.md

[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?

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.

JoshuaKGoldberg · 2024-09-03T21:41:59Z

package.json

@@ -27,6 +28,7 @@
    "!(*.{js,ts})": "prettier --write --ignore-unknown"
  },
  "devDependencies": {
+    "@types/mocha": "^10.0.7",


[Explanation] This isn't strictly necessary with "noImplicitAny": false. But it's just good to have types for dependencies in general.

packages/plugin-kit/src/config-comment-parser.js

JoshuaKGoldberg · 2024-09-03T21:49:28Z

scripts/build.js

- * 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


[Fix] Typo: missing > closing out Map.

JoshuaKGoldberg · 2024-09-03T21:50:33Z

tsconfig.base.json

+    // TODO: Over time, enable these strict options
+    "noImplicitAny": false,
+    "strictNullChecks": false,
+    "useUnknownInCatchVariables": false


[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?

I think strict vs. not should be opened up as a separate discussion.

types/levn.d.ts

.github/workflows/ci.yml

CONTRIBUTING.md

nzakas · 2024-09-04T15:11:32Z

CONTRIBUTING.md

+### Linting
+
+ESLint is linted using ESLint.
+[Building](#building) the project must be done before it can lint itself.


Did you find this to be true? Linting generally happens on source files, so not sure why building would be required first.

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 . $ |

packages/plugin-kit/src/config-comment-parser.js

types/levn.d.ts

Co-authored-by: Nicholas C. Zakas <[email protected]>

nzakas · 2024-09-26T15:50:52Z

package.json

@@ -8,6 +8,7 @@
    "build": "node scripts/build.js",
    "build:readme": "node tools/update-readme.js",
    "lint": "eslint .",
+    "tsc": "tsc",


We have some guidelines around script names, so I'd suggest this:

Suggested change

"tsc": "tsc",

"test:types": "tsc",

I'm not sure that this fits any of the existing molds. Walking through my thought process:

Lint: I think type checking is closest to "scripts that statically analyze files". But, TypeScript is not a linter, and users tend to get confused about the difference. I've seen confusion arise directly from labeling it as such.

Test: TypeScript doesn't execute code, and that's another (less) common confusion among users I've seen.

Suggestion: maybe "types" / "types:check" if you're looking for a more tool-agnostic name?

While I understand your analysis, I don't think this warrants a new script name. If running tsc is just meant to verify the types are correct, then test:types describes it perfectly.

nzakas · 2024-09-26T15:52:02Z

packages/plugin-kit/src/config-comment-parser.js

@@ -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)) || {};


Should the parens enclose the entire expression?

Suggested change

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

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

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 assertions¹ 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

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... ↩

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?

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.

Ah gotcha. Thanks for explaining 👍

chore: fully type check packages/*/src files

ae9c13c

eslint-github-bot bot added the chore label Sep 3, 2024

JoshuaKGoldberg commented Sep 3, 2024

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

JoshuaKGoldberg commented Sep 3, 2024

View reviewed changes

packages/plugin-kit/src/config-comment-parser.js Show resolved Hide resolved

JoshuaKGoldberg added 2 commits September 3, 2024 17:48

chore: remove unnecessary type assertion

cb49807

chore: remove unnecessary unknown/object type changes

b0cbf31

JoshuaKGoldberg commented Sep 3, 2024

View reviewed changes

types/levn.d.ts Outdated Show resolved Hide resolved

JoshuaKGoldberg marked this pull request as ready for review September 3, 2024 22:30

nzakas reviewed Sep 4, 2024

View reviewed changes

Update CONTRIBUTING.md

ebeb4c8

Co-authored-by: Nicholas C. Zakas <[email protected]>

JoshuaKGoldberg mentioned this pull request Sep 5, 2024

Repo: Enable TypeScript's "strict" mode #118

Open

JoshuaKGoldberg added 4 commits September 4, 2024 20:23

Remove strict mode

3e4c6e5

Merge branch 'main' into tsc-on-src

cb403a2

chore: use @types/levn

03bb82a

chore: split out type_check job

47d20c8

JoshuaKGoldberg requested a review from nzakas September 26, 2024 11:38

nzakas reviewed Sep 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

JoshuaKGoldberg commented Sep 3, 2024 •

edited

Loading

JoshuaKGoldberg Sep 3, 2024

nzakas Sep 5, 2024

JoshuaKGoldberg Sep 3, 2024

JoshuaKGoldberg Sep 3, 2024

JoshuaKGoldberg Sep 3, 2024

nzakas Sep 4, 2024

JoshuaKGoldberg Sep 5, 2024

nzakas Sep 4, 2024

JoshuaKGoldberg Sep 5, 2024

nzakas Sep 26, 2024

JoshuaKGoldberg Sep 27, 2024 •

edited

Loading

nzakas Sep 27, 2024

nzakas Sep 26, 2024

JoshuaKGoldberg Sep 27, 2024 •

edited

Loading

nzakas Sep 27, 2024

JoshuaKGoldberg Sep 27, 2024

nzakas Sep 27, 2024

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

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

Are you sure you want to change the base?

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

Conversation

JoshuaKGoldberg commented Sep 3, 2024 • edited Loading

Prerequisites checklist

What is the purpose of this pull request?

What changes did you make? (Give an overview)

Related Issues

Is there anything you'd like reviewers to focus on?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshuaKGoldberg Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshuaKGoldberg Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Footnotes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshuaKGoldberg commented Sep 3, 2024 •

edited

Loading

JoshuaKGoldberg Sep 27, 2024 •

edited

Loading

JoshuaKGoldberg Sep 27, 2024 •

edited

Loading