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

type-check reports errors twice #437

Open
unshame opened this issue Jan 31, 2024 · 16 comments · May be fixed by #549
Open

type-check reports errors twice #437

unshame opened this issue Jan 31, 2024 · 16 comments · May be fixed by #549
Labels
enhancement New feature or request

Comments

@unshame
Copy link

unshame commented Jan 31, 2024

Creating a project with typescript and vitest

Vue.js - The Progressive JavaScript Framework

√ Add TypeScript? ... Yes
√ Add Vitest for Unit Testing? ... Yes

making a mistake somewhere and then running type-check script reports the errors twice:

image

This is due to the fact that "src" is included in both tsconfig.app.json and tsconfig.vitest.json. This was introduced with #274

Not sure how to fix this. It's nice to have a separate tsconfig for vitest, but not sure it's worth it if it double reports the errors because of it.

@cexbrayat
Copy link
Member

Hi @unshame , thanks for the report.

I noticed it, but did not really mind personally.

@sodatea do you have an idea on how we could do better for this?

@sjihdazii
Copy link

sjihdazii commented Apr 22, 2024

@sodatea @unshame I have an idea what the cause may be. The tsconfig.json references the tsconfig.app.json and the tsconfig.vitest.json. The vue tsc build will run for both files. So if both tsconfig files include the file with the error; the error will be shown twice.

The solution could be changing the type check script in the package.json to: "type-check": "vue-tsc --build ./tsconfig.vitest.json --force",

@unshame
Copy link
Author

unshame commented Apr 23, 2024

@sjihdazii yeah, but that defeats the purpose of having two configs. The .vitest.json config has additional globals defined which are only available in the .test files.

@codethief
Copy link

codethief commented Jul 25, 2024

Hi there, I just stumbled over this, too.

Why does tsconfig.vitest.json need to include the production code (code-under-test) in the first place? Isn't this exactly the point of project references – to allow a split between different parts of the code base that need to run with different TSC settings & type declarations (production code needs client-side types, test code needs node & jsdom types etc.)?

In other words, how about changing tsconfig.vitest.json to something like

{
  "extends": "@vue/tsconfig/tsconfig.dom.json", // <--- No longer extend tsconfig.app.json
  "include": ["src/**/__tests__/**/*"], // <-- Include only tests
  "exclude": [],
  "compilerOptions": {
    "composite": true,
    "tsBuildInfoFile": "./node_modules/.tmp/tsconfig.vitest.tsbuildinfo",

    "lib": [],
    "types": ["node", "jsdom"]
  },
  "references": [
    { "path": "./tsconfig.app.json" }, // <-- Reference tsconfig.app.json to allow test files to import production code
  ],
}

Note that this requires setting something like

{
  "compilerOptions": {
    "noEmit": false,
    "emitDeclarationOnly": true,
  }
}

in tsconfig.app.json and configuring outDir appropriately (e.g. setting it ./node_modules/.tmp/app or something). Compare my comment here.

EDIT: I uploaded some example code here.

@unshame
Copy link
Author

unshame commented Jul 28, 2024

I was under the impression that vue-tsc didn't work with noEmit: false

Edit: oh, yeah, that works, cool

Edit2: seems not to work when importing .vue files in tests - I get a weird File X.vue.ts is not listed within the file list of project Y. Projects must list all files or use an 'include' pattern.. I assume it's the .vue.ts extension and the way vue-tsc handles vue files that's causing the issue.

And I'm unable to include just the .vue files with "include": ["./src/**/*.test.ts", "./src/**/*.vue"] or "include": ["./src/**/*.test.ts", "./src/**/*.vue.ts"]. Only "include": ["src"] works.

Edit3: this issue has been fixed in volar 2.0.26 vuejs/language-tools#3526 so looks like this solution works 🎉

@codethief
Copy link

@unshame I'm having trouble following you but I'm glad you found a fix!

For everyone else, I uploaded an example project the other day where I had tsconfig.vitest.json reference the production code instead of includeing it and things seemed to work fine.

@unshame
Copy link
Author

unshame commented Aug 1, 2024

@codethief I was updating the comment as I was trying to implement your suggestion, so it came out a bit incomprehensible. Your approach works with vue-tsc version 2 or later. That was my main point.

It also works for npm/pnpm workspaces as I found out - I've been able to utilize ts project references properly by putting the following in the package.json of each workspace:

{
  "exports": {
    "types": "./node_modules/.tmp/app/src/index.d.ts",
    "default": "./src/index.ts"
  }
}

I never realized I could just reference the compiled types while still referencing the uncompiled code. And this works in vscode & webstorm without needing to pre-compile anything.

This, with the vitest tsconfig optimization, has cut down typechecking times for my project significantly (to one minute from seven).

So huge thanks!

@unshame
Copy link
Author

unshame commented Aug 1, 2024

@cexbrayat would you consider using the approach @codethief proposed for this repo? I have some free time, so I can submit a PR.

@cexbrayat
Copy link
Member

Sure, we would be glad to get a PR and see if that improves the situation! 👍

@unshame
Copy link
Author

unshame commented Aug 4, 2024

@cexbrayat what's the reason for putting e2e tsconfigs in the e2e folder and not including them in the references of the main tsconfig? Is it because of this same issue by any chance?

@Tamas-hi
Copy link

I have followed a few threads, but I am now unsure what the current status is. As of today, I am still experiencing the same issue.

image

@haoqunjiang
Copy link
Member

haoqunjiang commented Nov 11, 2024

One thing I'm trying to experiment with now is to:

  • Remove composite: true from all tsconfig.*.json
  • Add "include": ["src/**/__tests__/*", "env.d.ts"], to tsconfig.vitest.json

With this configuration, files not imported into a .spec.ts would only report errors once. So, it mitigates the issue.
But I'm waiting for feedback from microsoft/TypeScript#60465 to ensure this change doesn't introduce any unexpected consequences.

@codethief
Copy link

codethief commented Nov 11, 2024

@haoqunjiang

  • Remove composite: true from all tsconfig.*.json
  • Add "include": ["src/**/tests/*"], to tsconfig.vitest.json

If you do that, test code will no longer be able to import production code. The solution then is to either

  • add the production code to the test code. (=> Overlap between the includes of both tsconfig.*.jsons => duplicate errors & confusion which tsconfig the IDE should use for any given file => back to square one.) Or to
  • add tsconfig.app.json to the references in tsconfig.vitest.json, in which case you do need the composite: true in tsconfig.app.json (see also my comment here).

I know the situation with project references is confusing and not at all well-documented – god knows they confused me for the longest time, too. However, I would suggest you take a closer look at the comments above, in particular, at the example code I shared and at @unshame's PR, since they already solve the present issue and use project references in the way they were intended.

@haoqunjiang
Copy link
Member

If you do that, test code will no longer be able to import production code.

Strictly speaking, the code imported to the spec files runs in the test environment, not the app code's browser environment.
So, I'm afraid cross-referencing may not be the ultimate solution here.

IMO, for components being unit-tested, it's not a bug for the errors to be reported twice. Because it means the types are wrong in both the browser and test environments.
But for code that is not unit-tested, say main.ts, it should only error once. That's what I'm trying to fix.

@codethief
Copy link

codethief commented Nov 11, 2024

Strictly speaking, the code imported to the spec files runs in the test environment, not the app code's browser environment.

That is very true. However, given that regular Vue code typically runs in a browser environment and test runners like Vitest emulate such an environment using JSDOM & friends, I'd say using project references in the way I outlined is a decent compromise for now. See also microsoft/TypeScript#37053 for the general discussion.

N.B. If you ask me, the underlying issue is the presence of a global scope in JS to begin with. If JS/TS had monads or algebraic effects to encode side effects, then we could save ourselves this gigantic pile of pain. (For instance, importing production code in test code would lead the type checker to verify automatically whether the test environment provides the appropriate "global" scope that the production code needs.)

@haoqunjiang
Copy link
Member

haoqunjiang commented Nov 11, 2024

IMO, for components being unit-tested, it's not a bug for the errors to be reported twice. Because it means the types are wrong in both the browser and test environments.

I just came up with a somewhat contrived example before going to bed. I'll write it down here and continue the discussion later.

Say the user is developing a web app utilizing the OPFS API, like https://github.com/diffusionstudio/vits-web
They might set "lib": ["ES2020", "DOM", "DOM.Iterable", "DOM.AsyncIterable"] in tsconfig.app.json.

Then, the following code, if present in HelloWorld.vue, would throw one distinct type error in each environment:

// Only error in browser environment
console.log(__dirname)

// Only error in vitest because jsdom doesn't implement File System API
// (`getDirectory`'s type inevitably sneaked in as jsdom references DOM lib;
// but jsdom doesn't opt into `DOM.AsyncIterable`, so `.entries` is never available)
const dirHandle = await navigator.storage.getDirectory()
console.log(dirHandle.entries())

haoqunjiang added a commit to haoqunjiang/create-vue that referenced this issue Dec 12, 2024
We are only using `references` in a solution-style tsconfig.
According to discussions at microsoft/TypeScript#60465,
such usage doesn't require `composite: true` to be set in sub-configs.

Removing this field loosens the constraints on these configs that all
files to be explicitly listed in `files` or `includes`.

After this change, type errors in source code would only be reported
twice if they're also imported by unit test specs, in constrast to
always be reported twice prior to the change.
I know this is not ideal yet, but it's still an improvement, and might
help catch some edge cases such as vuejs#437 (comment)

In the long run, we should still keep an eye on vuejs#549
(pending vuejs/language-tools#4750).
Cross-referencing might be a more intuitive configuration, and should be
the desirable configuration if we opt into Vitest Browser Mode.
haoqunjiang added a commit to haoqunjiang/create-vue that referenced this issue Dec 12, 2024
We are only using `references` in a solution-style tsconfig.
According to discussions at microsoft/TypeScript#60465,
such usage doesn't require `composite: true` to be set in sub-configs.

Removing this field loosens the constraints on these configs that all
files to be explicitly listed in `files` or `includes`.

After this change, type errors in source code would only be reported
twice if they're also imported by unit test specs, in contrast to
always be reported twice prior to the change.
I know this is not ideal yet, but it's still an improvement, and might
help catch some edge cases such as vuejs#437 (comment)

In the long run, we should still keep an eye on vuejs#549
(pending vuejs/language-tools#4750).
Cross-referencing might be a more intuitive configuration, and should be
the desirable configuration if we opt into Vitest Browser Mode.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants