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

refactor: unify TypeScript project #4404

Draft
wants to merge 16 commits into
base: master
Choose a base branch
from
Draft

refactor: unify TypeScript project #4404

wants to merge 16 commits into from

Conversation

turadg
Copy link
Member

@turadg turadg commented Jan 27, 2022

Description

I was trying to create a type coverage report and along the way it was easiest to unify the type checking into one TypeScript project. With this the checker can be more comprehensive and it turns out there are over a thousand type errors that were masked by the earlier config.

EDIT: having a single TypeScript project will solve OOM in eslint https://github.com/typescript-eslint/typescript-eslint/issues/1192#issuecomment-1153418862

Security Considerations

Documentation Considerations

Testing Considerations

@dckc
Copy link
Member

dckc commented Jan 28, 2022

Is typechecking still included in yarn lint after this PR? If not, note that much more loudly in the description, please. It would be a breaking change w.r.t. my mental model.

@mhofman
Copy link
Member

mhofman commented Jan 28, 2022

What is the advantage to moving the jsconfig.json to the root? Could the same be accomplished by having a base definition at the root and having each project extend it? My main concern is in the future being able to gradually move some packages to stricter rules.

@turadg
Copy link
Member Author

turadg commented Jan 28, 2022

Is typechecking still included in yarn lint after this PR? If not, note that much more loudly in the description, please. It would be a breaking change w.r.t. my mental model.

I've split that out to #4416

What is the advantage to moving the jsconfig.json to the root?

One is that we can now do yarn tsc at the root. I think there are other performance advantages for IDEs to check over one project instead of one per package.

Could the same be accomplished by having a base definition at the root and having each project extend it? My main concern is in the future being able to gradually move some packages to stricter rules.

I think there can still be overrides in sub-directories. I share the desire to have some packages go stricter sooner.

microsoft/TypeScript#28306 talks about this and here's an example of making nested configs work: microsoft/TypeScript#28306 (comment)

Though we might be better off on a per-file than per-package basis:
https://www.npmjs.com/package/typescript-strict-plugin?activeTab=readme

@mhofman
Copy link
Member

mhofman commented Jan 28, 2022

One is that we can now do yarn tsc at the root. I think there are other performance advantages for IDEs to check over one project instead of one per package.

yarn workspace should be able to handle running a single command at the root, right?

I'm also a little concerned this might hide some ambient style issues, were a package on its own might not be valid, but taken in combination with other packages would be fine.

I think there can still be overrides in sub-directories.

If the sub directory configs are automatically picked up and create a scope, that might be sufficient.

I think I still don't quite understand the motivation. I'd like to be able to run yarn lint:types or equivalent in a package and get a type check scoped to that package. In my mind that looks like a base jsconfig with the default compiler options in the root, and explicit jsconfig in every package with compiler options override (potentially none), and per package include / exclude as not all packages may have the same shape.

@turadg
Copy link
Member Author

turadg commented Feb 25, 2022

Noting this other motivation: to support refactoring. If we had a root project that others inherit from I think @warner wouldn't have run into the problem with jsconfig missing some files. I'm not sure but I suspect that VS Code will also be challenged by refactorings across packages since it sees each package as independent.

@mhofman
Copy link
Member

mhofman commented Feb 25, 2022

How would type checking a single package work with a root config? This is actually something I'm slightly frustrated at with the prettier change, that I cannot check the formatting when in packages/foo. Less of an issue for prettier since that's fast enough on the whole project, but it bugs me I need to keep doing cd or keep 2 terminals open.

@turadg
Copy link
Member Author

turadg commented Mar 1, 2022

How would type checking a single package work with a root config?

Each package config inherits the root config but still defines its own. So I don't think it would change the yarn lint:types ergonomics we have now.

This is actually something I'm slightly frustrated at with the prettier change, that I cannot check the formatting when in packages/foo. Less of an issue for prettier since that's fast enough on the whole project, but it bugs me I need to keep doing cd or keep 2 terminals open.

I had assumed that only CI would want to check Prettier and people would want to fix Prettier (since it's automatic and safe). If it's worth having a per-package check for it we can add that back. Personally I just have my IDE format to pretty on save.

New command, lint:all-types, that does a typecheck over the whole repo at once.
On my machine it takes 7s, versus minutes for typechecking within each package.

We want both to continue working so this leaves those alone and adds new
command that runs in the toplevel.

This isn't yet in CI.
Comment on lines -59 to +58
"better-sqlite3": "^8.2.0",
"better-sqlite3": "^9.1.1",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see discussion of some pretty fine details about sqlite among kernel devs. @gibson042 are you confident that this upgrade is OK? Or should it get review by others?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The major version bump was just to drop Node.js 16, so I'd be surprised if this affects anything not covered by unit tests. @warner gets deeper on this than I do, though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

runtime change so pulled out to #8590

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

Successfully merging this pull request may close these issues.

4 participants