Feature implementation from commits 8a18c8e..1e60c4f #2
Conversation
![codeowlai[bot]](https://avatars.githubusercontent.com/in/1259765?s=60&v=4){kind=small}
|
|
||
| public parseErrorStacktrace( | ||
| e: ErrorWithDiff, | ||
| e: TestError, |
There was a problem hiding this comment.
🐛 Correctness Issue
Type incompatibility with parent class method.
Changing parameter type from ErrorWithDiff to TestError may cause runtime errors if the parent class still expects ErrorWithDiff.
Current Code (Diff):
- e: TestError,
+ e: ErrorWithDiff,📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| e: TestError, | |
| e: ErrorWithDiff, |
🔄 Dependencies Affected
packages/browser/src/node/project.ts
Function: ProjectBrowser.parseErrorStacktrace
Issue: Method delegates to parent.parseErrorStacktrace with potentially incompatible parameter type
Suggestion: Ensure parent class accepts TestError type or revert to ErrorWithDiff
Proposed Code:
public parseErrorStacktrace(
e: ErrorWithDiff,
options: StackTraceParserOptions = {},
): ParsedStack[] {
return this.parent.parseErrorStacktrace(e, options)
}
| const { nextTick: safeNextTick } = globalThis.process || { | ||
| nextTick: cb => cb(), | ||
| } | ||
| const { nextTick: safeNextTick } = globalThis.process || {} |
There was a problem hiding this comment.
🐛 Correctness Issue
Removed nextTick fallback causes undefined function.
Removing the fallback implementation for nextTick will cause runtime errors when timers.nextTick is called in environments where process.nextTick is unavailable.
Current Code (Diff):
- const { nextTick: safeNextTick } = globalThis.process || {}
+ const { nextTick: safeNextTick } = globalThis.process || {
+ nextTick: cb => cb(),
+ }📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| const { nextTick: safeNextTick } = globalThis.process || {} | |
| const { nextTick: safeNextTick } = globalThis.process || { | |
| nextTick: cb => cb(), | |
| } |
| } from './json' | ||
|
|
||
| export const ReportersMap = { | ||
| 'default': DefaultReporter as typeof DefaultReporter, |
There was a problem hiding this comment.
🐛 Correctness Issue
Breaking API Change: Removed 'basic' Reporter Option.
Removing the 'basic' reporter option from ReportersMap will break any code that references this option, causing runtime exceptions.
| /** | ||
| * Exclude globs for test files | ||
| * @default ['**\/node_modules/**', '**\/dist/**', '**\/cypress/**', '**\/.{idea,git,cache,output,temp}/**', '**\/{karma,rollup,webpack,vite,vitest,jest,ava,babel,nyc,cypress,tsup,build,eslint,prettier}.config.*'] | ||
| * @default ['**\/node_modules/**', '**\/.git/**'] |
There was a problem hiding this comment.
🐛 Correctness Issue
Critical reduction in default test exclusion patterns.
Drastically reducing default exclude patterns will cause tests to run against build artifacts, config files, and temp directories, leading to unexpected behavior and performance issues.
Current Code (Diff):
- * @default ['**\/node_modules/**', '**\/.git/**']
+ * @default ['**\/node_modules/**', '**\/dist/**', '**\/cypress/**', '**\/.{idea,git,cache,output,temp}/**', '**\/{karma,rollup,webpack,vite,vitest,jest,ava,babel,nyc,cypress,tsup,build,eslint,prettier}.config.*']📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| * @default ['**\/node_modules/**', '**\/.git/**'] | |
| * @default ['**\/node_modules/**', '**\/dist/**', '**\/cypress/**', '**\/.{idea,git,cache,output,temp}/**', '**\/{karma,rollup,webpack,vite,vitest,jest,ava,babel,nyc,cypress,tsup,build,eslint,prettier}.config.*'] |
| globalThis.clearImmediate = currentClearImmediate | ||
|
|
||
| if (globalThis.process) { | ||
| if (globalThis.process && nextTick) { |
There was a problem hiding this comment.
🐛 Correctness Issue
Potential Runtime Exception: Unchecked Function Call.
The code attempts to call nextTick() without verifying it exists, which could cause a runtime exception if nextTick is undefined.
Current Code (Diff):
- if (globalThis.process) {
+ if (globalThis.process && nextTick) {📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| if (globalThis.process && nextTick) { | |
| if (globalThis.process && nextTick) { |
| private snapshotClient = getSnapshotClient() | ||
| private workerState = getWorkerState() | ||
| private __vitest_executor!: VitestExecutor | ||
| private __vitest_executor!: any |
There was a problem hiding this comment.
🐛 Correctness Issue
Type Safety Removal: VitestExecutor to any.
Changing the type from VitestExecutor to any removes compile-time type checking, which could lead to runtime crashes if executeId() method doesn't exist.
Current Code (Diff):
- private __vitest_executor!: any
+ private __vitest_executor!: VitestExecutor📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| private __vitest_executor!: any | |
| private __vitest_executor!: VitestExecutor |
🔄 Dependencies Affected
packages/vitest/src/runtime/runners/test.ts
Function: VitestTestRunner.importFile
Issue: Method relies on executeId() being available on __vitest_executor
Suggestion: Keep the strong typing to ensure executeId() method exists at compile time
Proposed Code:
async importFile(filepath: string) {
return this.__vitest_executor.executeId(filepath)
}
| continue | ||
| } | ||
|
|
||
| const [_, __, from] = (line.includes('with') |
There was a problem hiding this comment.
🐛 Correctness Issue
Unsafe regex destructuring could cause runtime errors.
The destructuring assignment doesn't handle the case where the regex doesn't match, potentially causing 'cannot read property of undefined' errors at runtime.
Current Code (Diff):
- const [_, __, from] = (line.includes('with')
- ? /Imported via '(.*)' from file '(.*)' with/.exec(line)
- : /Imported via '(.*)' from file '(.*)'/.exec(line)) ?? []
+ const match = line.includes('with')
+ ? /Imported via '(.*)' from file '(.*)' with/.exec(line)
+ : /Imported via '(.*)' from file '(.*)'/.exec(line)
+
+ if (!match) continue
+
+ const [_, __, from] = match📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| const [_, __, from] = (line.includes('with') | |
| const match = line.includes('with') | |
| ? /Imported via '(.*)' from file '(.*)' with/.exec(line) | |
| : /Imported via '(.*)' from file '(.*)'/.exec(line) | |
| if (!match) continue | |
| const [_, __, from] = match |
| @@ -0,0 +1,3 @@ | |||
| export function uncovered(condition: boolean) { | |||
| return condition ? 1 : 0 | |||
There was a problem hiding this comment.
🐛 Correctness Issue
Missing semicolon causing syntax error.
The return statement is missing a semicolon which will cause compilation failure in strict TypeScript environments.
Current Code (Diff):
- return condition ? 1 : 0
+ return condition ? 1 : 0;📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| return condition ? 1 : 0 | |
| return condition ? 1 : 0; |
| import type { TestUserConfig } from 'vitest/config' | ||
| import { assertType, describe, expectTypeOf, test } from 'vitest' | ||
| import { defineConfig, defineProject, defineWorkspace, mergeConfig } from 'vitest/config' | ||
| import { defineConfig, defineProject, mergeConfig } from 'vitest/config' |
There was a problem hiding this comment.
🐛 Correctness Issue
Removed defineWorkspace import breaks tests.
The defineWorkspace import was removed but it's still used in tests, which will cause compilation failures.
Current Code (Diff):
- import { defineConfig, defineProject, mergeConfig } from 'vitest/config'
+ import { defineConfig, defineProject, defineWorkspace, mergeConfig } from 'vitest/config'📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| import { defineConfig, defineProject, mergeConfig } from 'vitest/config' | |
| import { defineConfig, defineProject, defineWorkspace, mergeConfig } from 'vitest/config' |
🔄 Dependencies Affected
test/config-types.test-d.ts
Function: describe('define workspace helper')
Issue: Test block uses defineWorkspace but the import is removed
Suggestion: Keep the defineWorkspace import to maintain test functionality
| ...config.coverage, | ||
| provider: provider === 'v8-ast-aware' ? 'v8' : provider, | ||
| experimentalAstAwareRemapping: provider === 'v8-ast-aware', | ||
| provider, |
There was a problem hiding this comment.
🐛 Correctness Issue
Breaking change: v8-ast-aware provider handling removed.
Removing special handling for 'v8-ast-aware' provider will break tests that rely on this provider being transformed to 'v8' with experimentalAstAwareRemapping enabled.
Current Code (Diff):
- provider,
+ provider: provider === 'v8-ast-aware' ? 'v8' : provider,
+ experimentalAstAwareRemapping: provider === 'v8-ast-aware',📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| provider, | |
| provider: provider === 'v8-ast-aware' ? 'v8' : provider, | |
| experimentalAstAwareRemapping: provider === 'v8-ast-aware', |
PR Summary
Comprehensive Type System Refactoring and Deprecated API Removal
Overview
This PR performs a major refactoring of Vitest's type system, removing deprecated APIs, renaming types for consistency, and improving type safety across the codebase. It also includes significant changes to the coverage configuration system and fixes several timer-related issues.
Change Types
defineWorkspace,BasicReporter, and several type definitionsAffected Modules
vitest/src/types/ErrorWithDiff→TestError,UserConfig→TestUserConfig)vitest/src/public/vitest/src/node/coverage.tsvitest/src/defaults.tsbrowser/src/utils/src/timers.tsBreaking Changes
defineWorkspacefunction (useprojectsfield in root config instead)BasicReporterfrom reporter optionsUserConfigtoTestUserConfigfor clarityErrorWithDifftype in favor ofTestErrorextensionandalloptionsNotes for Reviewers
anyorMap<string, any>which reduces type safety but appears necessary for compatibilityAdditional Context
defineWorkspaceis a planned deprecation mentioned in TODO comments