Feature implementation from commits 41a111c..bdd2e01#1
Feature implementation from commits 41a111c..bdd2e01#1yashuatla wants to merge 15 commits intofeature-base-1from
Conversation
![codeowlai[bot]](https://avatars.githubusercontent.com/in/1259765?s=60&v=4){kind=small}
| ServerMockResolution, | ||
| ServerResolverOptions, | ||
| } from './resolver' | ||
| export type { ServerResolverOptions } from './resolver' |
There was a problem hiding this comment.
🐛 Correctness Issue
Breaking API Change: Removed Type Exports.
Removing ServerIdResolution and ServerMockResolution type exports will break any external code that imports these types, causing compilation failures.
Current Code (Diff):
- export type { ServerResolverOptions } from './resolver'
+ export type { ServerIdResolution, ServerMockResolution, ServerResolverOptions } from './resolver'📝 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! 🚀
| export type { ServerResolverOptions } from './resolver' | |
| export type { ServerIdResolution, ServerMockResolution, ServerResolverOptions } from './resolver' |
| */ | ||
| ignoreClassMethods?: string[] | ||
| } | ||
| export interface CoverageV8Options extends BaseCoverageOptions {} |
There was a problem hiding this comment.
🐛 Correctness Issue
Breaking API change: Removed V8 coverage options.
Removing specific V8 coverage options will break existing code that relies on these properties, causing compilation failures for consumers of this library.
Current Code (Diff):
+ export interface CoverageV8Options extends BaseCoverageOptions {}
- export interface CoverageV8Options extends BaseCoverageOptions {
- /**
- * Ignore empty lines, comments and other non-runtime code, e.g. Typescript types
- * - Requires `experimentalAstAwareRemapping: false`
- */
- ignoreEmptyLines?: boolean
-
- /**
- * Remap coverage with experimental AST based analysis
- * - Provides more accurate results compared to default mode
- */
- experimentalAstAwareRemapping?: boolean
-
- /**
- * Set to array of class method names to ignore for coverage.
- * - Requires `experimentalAstAwareRemapping: true`
- *
- * @default []
- */
- ignoreClassMethods?: string[]
- }📝 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! 🚀
| export interface CoverageV8Options extends BaseCoverageOptions {} | |
| export interface CoverageV8Options extends BaseCoverageOptions { | |
| /** | |
| * Ignore empty lines, comments and other non-runtime code, e.g. Typescript types | |
| * - Requires `experimentalAstAwareRemapping: false` | |
| */ | |
| ignoreEmptyLines?: boolean | |
| /** | |
| * Remap coverage with experimental AST based analysis | |
| * - Provides more accurate results compared to default mode | |
| */ | |
| experimentalAstAwareRemapping?: boolean | |
| /** | |
| * Set to array of class method names to ignore for coverage. | |
| * - Requires `experimentalAstAwareRemapping: true` | |
| * | |
| * @default [] | |
| */ | |
| ignoreClassMethods?: string[] | |
| } |
| * - Provides more accurate results compared to default mode | ||
| */ | ||
| experimentalAstAwareRemapping?: boolean | ||
| export interface CoverageIstanbulOptions extends BaseCoverageOptions {} |
There was a problem hiding this comment.
🐛 Correctness Issue
Duplicate interface declaration.
Creating a duplicate declaration of CoverageIstanbulOptions will cause TypeScript compilation errors due to ambiguous interface definitions.
Current Code (Diff):
- export interface CoverageIstanbulOptions extends BaseCoverageOptions {}
| globalThis.clearImmediate = currentClearImmediate | ||
|
|
||
| if (globalThis.process) { | ||
| if (globalThis.process && nextTick) { |
There was a problem hiding this comment.
🐛 Correctness Issue
Potential Runtime Exception: Missing null check for nextTick.
The code attempts to call nextTick() without verifying it exists, which would cause a runtime crash if nextTick is undefined in certain environments.
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 critical compile-time type checking, which could lead to runtime exceptions if the object doesn't have the expected methods.
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
Function: VitestTestRunner.importFile
Issue: The executeId method call on __vitest_executor loses type checking, risking runtime errors
Suggestion: Maintain the VitestExecutor type to ensure type safety for the executeId method call
| export interface TypecheckResults { | ||
| files: File[] | ||
| sourceErrors: TypeCheckError[] | ||
| sourceErrors: TestError[] |
There was a problem hiding this comment.
🐛 Correctness Issue
Type incompatibility: TypeCheckError[] to TestError[].
Changing sourceErrors from TypeCheckError[] to TestError[] may cause type incompatibilities if these types have different structures or if code elsewhere expects TypeCheckError specifically.
Current Code (Diff):
- sourceErrors: TestError[]
+ sourceErrors: TypeCheckError[] | TestError[]📝 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! 🚀
| sourceErrors: TestError[] | |
| sourceErrors: TypeCheckError[] | TestError[] |
🔄 Dependencies Affected
packages/vitest/src/typecheck/typechecker.ts
Function: Typechecker.constructor
Issue: The Typechecker class initializes _result with an empty sourceErrors array, but code adding to this array must now add TestError objects instead of TypeCheckError objects
Suggestion: Ensure all code that adds to sourceErrors uses TestError objects or update the type to support both TypeCheckError and TestError
| export function uncovered(condition: boolean) { | ||
| return condition ? 1 : 0 |
There was a problem hiding this comment.
🐛 Correctness Issue
Breaking API Change: Required Parameter Added.
Adding a required parameter to the function signature will break all existing callers that don't provide this parameter.
Current Code (Diff):
- export function uncovered(condition: boolean) {
- return condition ? 1 : 0
+ export function uncovered(condition: boolean = false) {
+ 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! 🚀
| export function uncovered(condition: boolean) { | |
| return condition ? 1 : 0 | |
| export function uncovered(condition: boolean = false) { | |
| 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 required defineWorkspace import.
The defineWorkspace function is removed from imports but is still used in tests, which will cause compilation errors.
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: define workspace helper test section
Issue: Tests for defineWorkspace helper will fail because the function is no longer imported
Suggestion: Keep the defineWorkspace import to ensure tests can access this function
4 participants
PR Summary
Type System Cleanup and Refactoring
Overview
This PR performs a comprehensive cleanup of the type system across the Vitest codebase, removing deprecated types, renaming types for consistency, and fixing type assertions. It also consolidates type definitions and improves import paths.
Change Types
Affected Modules
vitest/src/publicvitest/src/typesbrowser/srccoverage-v8/srcmocker/srcrunner/srcvitest/src/nodeBreaking Changes
defineWorkspacefunction in favor of direct array exportsErrorWithDiff,Custom,TaskContext,ExtendedContext,WorkspaceProjectConfigurationUserConfigtoTestUserConfigfor clarityclassnameoption from JUnit reporter in favor ofclassnameTemplatecontextIdproperty from BrowserCommandContext interface (replaced bysessionId)Notes for Reviewers
anyor specific types were added with TODO comments