[engsys] upgrade vitest dev dependencies to v4 #36373
Conversation
replace @vitest/browser with @vitest/browser-playwright
we only ever import and use them as types.
…onfig.json - dev-tool: apply strip-json-comments to tsconfig file contents before parsing. Usually tsconfig.json files allow comments.
because today
- Some of our packages have mix NodeJS/browser usage in same file
- TypeScript `NodeNext` module resolution uses ESM version of built .d.ts files
which may contain pattern of `import { ... } from "node:xxx";`
- Our build-test doesn't do platform file substitution yet so ESM version of
files are included in the build.
Vitest v3 pulls in NodeJS types implicitly but it is no longer the case in
vitest v4.
jeremymeng
requested review from
a team,
JacksonWeber,
Karlie-777,
MSNev,
MaryGao,
benbp,
deyaaeldeen,
hectorhdzg,
maorleger,
mikeharder,
qiaozha,
rads-1996,
timovv and
weshaggard
as code owners
github-actions
bot
added
Purview
Schema Registry
Search
Service Bus
Storage
jeremymeng
requested review from
Haiying-MSFT,
KevinBlasko,
ToddKingMSFT,
ms-meya,
nasundar and
qing-zhao
github-project-automation
bot
added this to
Azure Identity SDK Improvements and Azure SDK for Key Vault
github-project-automation
bot
moved this to Untriaged
in Azure Identity SDK Improvements
jeremymeng
requested review from
Ohad-Fein,
Ronnysi,
ischrei,
jairmyree,
joshfree,
olterner and
pvaneck
There was a problem hiding this comment.
Pull Request Overview
This PR upgrades the vitest development dependencies from v3 to v4 across the repository. The main changes include updating the @vitest/browser package to @vitest/browser-playwright to align with vitest v4's browser testing approach, and addressing type compatibility issues that arose from this upgrade.
Key changes:
- Upgrade vitest and related packages to v4.0.0
- Replace
@vitest/browserwith@vitest/browser-playwrightacross all packages - Fix type exports in event-hubs and service-bus packages to use
typeimports - Add JSON comment stripping support in dev-tool's TypeScript config resolver
Reviewed Changes
Copilot reviewed 299 out of 425 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-workspace.yaml | Update vitest catalog versions from v3.2.3 to v4.0.0 and rename browser package |
| sdk/*/package.json (300+ files) | Replace @vitest/browser with @vitest/browser-playwright in devDependencies |
| sdk/eventhub/event-hubs/src/index.ts | Change WebSocketImpl export to type-only export |
| common/tools/dev-tool/src/util/resolveTsConfig.ts | Add stripJsonComments import and usage for parsing TypeScript configs |
| eng/tools/ci-runner/package.json | Update vitest dependencies to v4.0.0 |
| eng/containers/turborepo-remote-cache/package.json | Update vitest dependencies to v4.0.0 |
| common/tools/eslint-plugin-azure-sdk/package.json | Remove unused @vitest/browser dependency |
|
|
||
| export { EventData, ReceivedEventData } from "./eventData.js"; | ||
| export { WebSocketImpl } from "rhea-promise"; | ||
| export type { WebSocketImpl } from "rhea-promise"; |
There was a problem hiding this comment.
Changing from a value export to a type-only export is correct for types that are only used in type positions. However, ensure that all consumer code only uses WebSocketImpl as a type and not as a value. If any code attempts to use it as a constructor or value, this will cause runtime errors.
| export type { WebSocketImpl } from "rhea-promise"; | |
| export { WebSocketImpl } from "rhea-promise"; |
There was a problem hiding this comment.
No it is an interface, a re-export from rhea
There was a problem hiding this comment.
with that said, it appears to be a regression that vitest v4 is not able to strip this type-only import.
| "types": [], | ||
| // Workaround for browser build-test: | ||
| // - Some core and storage package have mixed usage of NodeJS and browser types in same file. | ||
| // - TypeScript resolves our types from ESM because of `"moduleResolution": "NodeNext"` |
There was a problem hiding this comment.
can we change module resolution to be bundler?
There was a problem hiding this comment.
That only affects whether .js extension is required for relative import
'bundler' for use with bundlers. Like node16 and nodenext, this mode supports package.json "imports" and "exports", but unlike the Node.js resolution modes, bundler never requires file extensions on relative paths in imports.
API Change CheckAPIView identified API level changes in this PR and created the following API reviews |
bump version to v4 and add @vitest/browser-playwright to dev deps per reference: https://github.com/vitest-dev/vitest/blob/main/docs/guide/migration.md
event-hubs & service-bus: fix export of Delivery and WebSocketImpl. We only ever import and use them as types
root tsconfig.browser.config.json workaround: need NodeJS types for browser typechecking because today
NodeNextmodule resolution uses ESM flavor of our built .d.ts files which may contain pattern ofimport { ... } from "node:xxx";vitest v3 pulls in NodeJS types implicitly so it works now but it is no longer the same in vitest v4.
test-utils: remove unused multi version testing support