chore(deps): update Vite to 6.3.5 #8051
Conversation
✅ Deploy Preview for vitest-dev ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
I don't think Vite update should affect how vue coverage is collected |
|
Ah, no. The AST-aware coverage feature was just between them. |
|
It seems it starts failing with Vite 6.3.3. Probably vitejs/vite#19841 or vitejs/vite#19842 |
|
It seems vitejs/vite#19842 cc @hi-ogawa |
test/coverage-test/test/vue.test.ts
Outdated
| else if (isExperimentalV8Provider()) { | ||
| expect(coverageMap).toMatchInlineSnapshot(` | ||
| { | ||
| "branches": "6/8 (75%)", | ||
| "functions": "5/7 (71.42%)", | ||
| "lines": "14/17 (82.35%)", | ||
| "statements": "15/18 (83.33%)", | ||
| } | ||
| `) | ||
| } |
There was a problem hiding this comment.
v8-ast-aware shows +1 line/statement for default export statement compared to istanbul, but this seems more correct to me and I'm not sure it's a bad thing 🤔
Istanbul (Left)
- Statements 3/5
- Lines 3/5
v8-ast-awawe (Right)
- Statements 4/6
- Lines 4/6
@AriPerkkio I think export default didn't show up on v8-ast-aware coverage because it matches __vite_ssr_exports__.default = ... of ignoreNode, but is that intended other than for the sake of istanbul 100% matches?
vitest/packages/coverage-v8/src/provider.ts
Lines 275 to 285 in 0cbad1b
Btw, export default is now transformed to const __vite_ssr_export_default__ = ..., so I changed ignoreNode, this didn't have effect.
if (
node.type === 'VariableDeclaration'
&& node.declarations.length === 1
&& node.declarations[0].id.type === 'Identifier'
&& node.declarations[0].id.name === '__vite_ssr_export_default__'
) {
return true
}The reason why I think new coverage is correct is that, this now treats following two statements as covered equally.
export const foo = defineComponent({ name: 'Foo' }) // covered
export default defineComponent({ name: 'bar' }) // coveredIstanbul and previous v8-ast-aware doesn't say export default to be covered and this seems like an issue of Istanbul.
export const foo = defineComponent({ name: 'Foo' }) // covered
export default defineComponent({ name: 'bar' }) // not covered
There was a problem hiding this comment.
This should match 100% with Istanbul. Export-statements should not be included in coverage.
In this example:
export const foo = defineComponent({ name: 'Foo' }) // covered
export default defineComponent({ name: 'bar' }) // not covered... the first one is a assignment and is included in coverage report. Second one is just export, and is not included. You should see this in practise with:
const foo = defineComponent({ name: 'Foo' }) // covered
export { foo } // Not coveredThere was a problem hiding this comment.
That's interesting, but I still don't comprehend the rational or underlying motivation. Why would users expect the behavior like this?
defineComponent({ name: 'bar' }) // covered
export default defineComponent({ name: 'bar' }) // not coveredThere was a problem hiding this comment.
I think there's a subtle nuance in export . I'm wondering what's the expected behavior of the following
export function f1() {} // not covered
export default function f2() {} // not covered
export const f3 = function f3() {} // covered
export default (function f4() {}) // covered?Based on vitejs/vite#19834, semantically we can see 4th example as:
const __internal__ = function f4() {}
export { __internal__ as default }so making it covered doesn't look too wrong technically.
There was a problem hiding this comment.
Looks like that case is intentionally ignored: https://github.com/istanbuljs/istanbuljs/blob/06eec782dc8a248f0516cdba06b280c410515890/packages/istanbul-lib-instrument/src/visitor.js#L636
ExpressionStatement though is tracked.
There was a problem hiding this comment.
@hi-ogawa I do agree with you that this is something that should likely end up in the coverage report. If we can get this fixed in istanbul-lib-instrument upstream, then we could do same for V8 in Vitest. For now we want 100% match between the two.
There was a problem hiding this comment.
For now we want 100% match between the two.
Sounds good 👍
Btw, I think there's one more edge case with export default. The transform can be seen in Vite test cases https://github.com/vitejs/vite/blob/54e0a18773be6f6c35d92db5be7f3e6159c23589/packages/vite/src/node/ssr/__tests__/ssrTransform.spec.ts#L518-L556 and the basic idea is:
export default function f() {}
// ⇓
Object.defineProperty(..., "default", ... f ...)
function f() {} export default class A {}
// ⇓
Object.defineProperty(..., "default", ... A ...)
class A {}export default otherCase
// ⇓
Object.defineProperty(..., "default", ... __vite_ssr_export_default__ ...)
const __vite_ssr_export_default__ = otherCaseThe last case includes anonymous function and class. Probably this case needs to be kept "not covered" on our side even if istanbul changes the behavior of export default expr.
export default function () [};
// ⇓
Object.defineProperty(..., "default", ... __vite_ssr_export_default__ ...)
const __vite_ssr_export_default__ = function () {}
|
I'm setting this as draft for a while - we need some adjustments for coverage. From Vite's side everything is OK. |
|
Thanks for fixing! |
4 participants
Description
Updated Vite to fix the ecosystem-ci failure.
https://github.com/vitejs/vite-ecosystem-ci/actions/runs/15291891294/job/43012757806#step:8:5112
I've updated the snapshot, but I'm not sure if the change correct.
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Please, don't make changes topnpm-lock.yamlunless you introduce a new test example.Tests
pnpm test:ci.Documentation
pnpm run docscommand.Changesets
feat:,fix:,perf:,docs:, orchore:.