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

Feat project references #549

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

Conversation

unshame
Copy link

@unshame unshame commented Aug 4, 2024

Description

Fixes duplicate typechecking and error reporting when running type-check with vitest or cypress component tests enabled. This should also speed up typechecking.

Adds usage of project references to vitest and cypress component test tsconfigs. Adds declaration emit to the tsconfig.app.json. Only includes test files in unit testing tsconfigs.

Fixes #437

Running pnpm run type-check in typescript-vitest after:

image
image

Running pnpm run type-check in typescript-vitest before (2 duplicate errors):

image

Note: due to the fact that typescript stops checking if a project reference has errors, the errors in test files appear only after errors in the app are fixed (when running vue-tsc, it doesn't affect how IDEs display errors). There's an upcoming typescript feature that will fix this, but I don't think it's a big deal even without the fix. Definitely better than having errors reported twice.

Thanks to @codethief for the idea.

E2E tests

I didn't touch tsconfigs for e2e tests. They most likely can be setup the same way as unit tests, I don't see why they need to be in a separate folder. There are more changes that need to be made to achieve this though, that should be done in a separate PR.

The nightwatch config is the weirdest case - there is just a copy of tsconfig.app.json in it that overwrites the original one. I tried to set it up to work the same way as cypress-ct does, but looks like the vue plugin doesn't have any types, so it was giving me errors. I deleted that instead of copying the new content of tsconfig.app.json into it.

@@ -1,11 +1,18 @@
{
"extends": "./tsconfig.app.json",
Copy link
Author

Choose a reason for hiding this comment

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

I kept this extending the app config, but maybe it's better to make it extend @vue/tsconfig/tsconfig.dom.json directly? That's how I've set it up in my project, makes it easier to manage.

Choose a reason for hiding this comment

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

Yeah, I'd argue it's better to keep production and test configs separate (and rather have them extend a common base config), so that people won't e.g. add types to tsconfig.app.json that don't belong in test files.

"cypress/support/component.*",
"cypress/support/commands.ts"
],
"exclude": [],
"compilerOptions": {
"composite": true,
"outDir": "./node_modules/.tmp/cypress-ct",
Copy link
Author

Choose a reason for hiding this comment

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

It's not strictly necessary to have the declarations output for anything but the tsconfig.app.json right now, but I think it makes sense to make them all work the same way.

Copy link

@codethief codethief Aug 5, 2024

Choose a reason for hiding this comment

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

Agreed. Besides, whether or not declarations will be output will change slightly in TypeScript 5.6, see microsoft/TypeScript#32651 (comment)

@cexbrayat
Copy link
Member

Let's hear from @sodatea what he thinks about this setup

"exclude": [],
"compilerOptions": {
"composite": true,
"outDir": "./node_modules/.tmp/vitest",
"tsBuildInfoFile": "./node_modules/.tmp/tsconfig.vitest.tsbuildinfo",
Copy link

@codethief codethief Aug 5, 2024

Choose a reason for hiding this comment

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

tsBuildInfoFile can be removed since now that we set outDir, its default value will be <outDir>/<config name>.tsbuildInfo, compare https://www.typescriptlang.org/tsconfig/#tsBuildInfoFile

Same thing goes for tsBuildInfoFile in the other tsconfigs.

@codethief
Copy link

Thanks so much for picking this up, @unshame ! :)

@codethief
Copy link

codethief commented Aug 21, 2024

@unshame Since our conversation in #437, I have run into vuejs/language-tools#3526 with increasing frequency. Right now it blocks me from introducing project references in the frontend project at my day job. I'd therefore be hesitant to merge this PR before the upstream issue gets fixed. :\

@unshame
Copy link
Author

unshame commented Aug 21, 2024

Interesting.

I do see the error in vscode:
image

Webstorm on the other hand has no issues with it and it's using the same version of vue language server.
image
image

I also don't have issues with vue-tsc, it reports the errors correctly and doesn't infer the type of the component as any:
image

So looks like a vscode extension bug?

@codethief
Copy link

@unshame Interesting. My coworker has been reporting issues at least in IntelliJ, though. Would you mind re-posting your comment in vuejs/language-tools#3526 together with the exact versions of vue-tsc, VSCode extension, WebStorm, WebStorm Vue extension etc.?

@messenjer
Copy link
Contributor

They just released a new version of volar and vue-tsc (2.4.0) : https://github.com/volarjs/volar.js/releases/tag/v2.4.0

@codethief
Copy link

codethief commented Aug 21, 2024

@messenjer vue-tsc is still at 2.0.29, see https://www.npmjs.com/package/vue-tsc . I don't know which Volar version vue-tsc 2.0.29 uses¹ but it was released ~a month ago. (¹ There are no release notes and no git tag for 2.0.29 on https://github.com/vuejs/language-tools yet.)

Meanwhile, @vuejs/language-tools master updated to Volar 2.4.0 only 2 days ago. So we'll have to wait for a new vue-tsc version that uses Volar 2.4.0.

In any case, let's please continue this discussion in vuejs/language-tools#3526.

@codethief
Copy link

codethief commented Sep 16, 2024

Since my last comments a lot has happened: One ticket has been closed, another (new) ticket is still open. Personally, I decided the remaining issue is no longer a blocker, since it only affects IDE support in minor ways and type checking is not affected. So in our project I went ahead and implemented project references throughout and until now I haven't received any major complaints from my coworkers.

For create-vue the considerations might be different, though, I don't know.

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.

type-check reports errors twice
4 participants