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.

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

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

Conversation

JoshuaKGoldberg
Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg commented Sep 3, 2024

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

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.

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

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

tsconfig.base.json Outdated Show resolved Hide resolved
types/levn.d.ts Outdated Show resolved Hide resolved
@JoshuaKGoldberg JoshuaKGoldberg marked this pull request as ready for review September 3, 2024 22:30
.github/workflows/ci.yml Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
### 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 .

$ |

types/levn.d.ts Outdated Show resolved Hide resolved
Co-authored-by: Nicholas C. Zakas <[email protected]>
package.json Outdated Show resolved Hide resolved
@@ -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

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 👍

@nzakas
Copy link
Member

nzakas commented Oct 18, 2024

@JoshuaKGoldberg are you still working on this?

@JoshuaKGoldberg
Copy link
Contributor Author

Yes! I'd been waiting on #118 to get resolved. Cleaning this up now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Implementing
Development

Successfully merging this pull request may close these issues.

3 participants