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

fix(typechecker): reject when tsc fails to start #6561

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

Conversation

mmkal
Copy link
Contributor

@mmkal mmkal commented Sep 24, 2024

Description

Give a better error message when trying to use typecheck without typescript installed.

Previously, it was working ok with pnpm vitest run but just hanging on pnpm vitest (the TS task never completed). This was because we weren't listening for the 'error' event, and set throwOnError: false when spawning the process.

The fix was to have a Promise.race between the process spawn event and the error event, with the error event rejecting with a helpful message.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • _ there isn't an issue for this exactly, but I talked about it with @sheremet-va _ It's really useful if your PR references an issue where it is discussed ahead of time. If the feature is substantial or introduces breaking changes without a discussion, PR might be closed.
  • Ideally, include a test that fails without this PR but passes with it.
  • Please, don't make changes to pnpm-lock.yaml unless you introduce a new test example.

Tests

  • Run the tests with pnpm test:ci.

Documentation

  • If you introduce new functionality, document it. You can run documentation with pnpm run docs command.

Changesets

  • Changes in changelog are generated from PR name. Please, make sure that it explains your changes in an understandable manner. Please, prefix changeset messages with feat:, fix:, perf:, docs:, or chore:.

Copy link

netlify bot commented Sep 24, 2024

Deploy Preview for vitest-dev ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 00e70a8
🔍 Latest deploy log https://app.netlify.com/sites/vitest-dev/deploys/670834fbb86bc4000852a81f
😎 Deploy Preview https://deploy-preview-6561--vitest-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Sep 26, 2024

Thanks for the PR! Indeed this hangs on main currently:

$ pnpm -C examples/typecheck test -- --typecheck.checker tsc-bad
...
 DEV  v2.1.1 /home/hiroshi/code/others/vitest/examples/typecheck

 ·  TS  test/type.test-d.ts (1)
 ✓ test/normal.test.ts (1)

I'm testing your PR locally and while it shows a better error, I'm seeing a weird hanging process node (vitest 1) which eventually OOM. On my linux PC, this is how my terminal looks like:

$ pnpm -C examples/typecheck test -- --typecheck.checker tsc-bad
...
 DEV  v2.1.1 /home/hiroshi/code/others/vitest/examples/typecheck


⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ Unhandled Rejection ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Error: Spawning typechecker failed - is typescript installed?
 ❯ ChildProcess.<anonymous> ../../packages/vitest/dist/chunks/RandomSequencer.C4LDd-jj.js:879:16
 ❯ Object.onceWrapper node:events:634:26
 ❯ ChildProcess.emit node:events:531:35
 ❯ ChildProcess._handle.onexit node:internal/child_process:292:12
 ❯ onErrorNT node:internal/child_process:484:16
 ❯ process.processTicksAndRejections node:internal/process/task_queues:82:21

Caused by: Error: spawn tsc-bad ENOENT
 ❯ ChildProcess._handle.onexit node:internal/child_process:286:19
 ❯ onErrorNT node:internal/child_process:484:16
 ❯ process.processTicksAndRejections node:internal/process/task_queues:82:21

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { errno: -2, code: 'ENOENT', syscall: 'spawn tsc-bad', path: 'tsc-bad', spawnargs: [ '--noEmit', '--pretty', 'false', '-p', '/home/hiroshi/code/others/vitest/examples/typecheck/tsconfig.vitest-temp.json', '--watch' ] }

$ ... wait for a while after terminal is back ...

<--- Last few GCs --->

[37137:0x438b1840]    41388 ms: Mark-Compact 4043.2 (4139.0) -> 4030.9 (4141.8) MB, 560.84 / 1.08 ms  (average mu = 0.253, current mu = 0.049) allocation failure; scavenge might not succeed
[37137:0x438b1840]    42441 ms: Mark-Compact 4046.8 (4141.8) -> 4034.4 (4145.5) MB, 1021.84 / 0.60 ms  (average mu = 0.120, current mu = 0.029) allocation failure; scavenge might not succeed


<--- JS stacktrace --->

FATAL ERROR: Reached heap limit Allocation failed - JavaScript heap out of memory
----- Native stack trace -----

 1: 0xb86ecf node::OOMErrorHandler(char const*, v8::OOMDetails const&) [node (vitest 1)]
 2: 0xef74d0 v8::Utils::ReportOOMFailure(v8::internal::Isolate*, char const*, v8::OOMDetails const&) [node (vitest 1)]
 3: 0xef77b7 v8::internal::V8::FatalProcessOutOfMemory(v8::internal::Isolate*, char const*, v8::OOMDetails const&) [node (vitest 1)]
 4: 0x1109355  [node (vitest 1)]
 5: 0x11211d8 v8::internal::Heap::CollectGarbage(v8::internal::AllocationSpace, v8::internal::GarbageCollectionReason, v8::GCCallbackFlags) [node (vitest 1)]
 6: 0x10f72f1 v8::internal::HeapAllocator::AllocateRawWithLightRetrySlowPath(int, v8::internal::AllocationType, v8::internal::AllocationOrigin, v8::internal::AllocationAlignment) [node (vitest 1)]
 7: 0x10f8485 v8::internal::HeapAllocator::AllocateRawWithRetryOrFailSlowPath(int, v8::internal::AllocationType, v8::internal::AllocationOrigin, v8::internal::AllocationAlignment) [node (vitest 1)]
 8: 0x10d5ad6 v8::internal::Factory::NewFillerObject(int, v8::internal::AllocationAlignment, v8::internal::AllocationType, v8::internal::AllocationOrigin) [node (vitest 1)]
 9: 0x1531906 v8::internal::Runtime_AllocateInYoungGeneration(int, unsigned long*, v8::internal::Isolate*) [node (vitest 1)]
10: 0x76d50d4d9ef6 

Do you see something similar?

Also I noticed tsconfig.vitest-temp.json is not cleaned up currently.

@mmkal
Copy link
Contributor Author

mmkal commented Sep 26, 2024

Thanks for the tip, have removed the Promise.race.

Re the OOM - yes I was able to reproduce that just now. But, I was also able to reproduce it on main, so not sure it's related - but it's possible that this change makes it more frequent or something?

Re tsconfig.vitest-temp.json - reproing that too with your command. It looks like we only call typechecker.clear() when the startup promise doesn't reject. Will look into whether there's a clean way to always call it.

@mmkal
Copy link
Contributor Author

mmkal commented Sep 26, 2024

Resolved the temp file issue by explicitly calling await this.clear() before rejecting. I don't love that as a solution but what I originally tried, which was calling .clear() from packages/vitest/src/node/pools/typecheck.ts - didn't seem to work. Maybe that's not what manages the typechecker?

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Sep 27, 2024

Re the OOM - yes I was able to reproduce that just now. But, I was also able to reproduce it on main, so not sure it's related - but it's possible that this change makes it more frequent or something?

Thanks for confirming. I don't fully understand why, but it looks like this startTypechecker is not awaited here and this might be making a situation worse:

startTypechecker(project, files)

If I do promises.push(startTypechecker(project, files)) here, it's looking better though not sure if it's breaking something else. Can you try it and see how CI goes?

Copy link
Contributor

@hi-ogawa hi-ogawa left a comment

Choose a reason for hiding this comment

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

Can you add test in test/typescript? Not the realistic one, but verifying something like { typecheck: { checker: "bad-checker" }} and stderr should be more than enough.

Comment on lines +334 to +337
if (startupResult) {
await this.clear()
throw new Error('damn', { cause: startupResult })
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this.clear is not needed anymore since pool gets closed properly now.

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.

3 participants