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: call resolveId('vitest') after buildStart #5646

Merged
merged 8 commits into from
May 2, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions packages/vitest/src/node/core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,11 +110,6 @@ export class Vitest {

this.vitenode = new ViteNodeServer(server, this.config.server)

// if Vitest is running globally, then we should still import local vitest if possible
const projectVitestPath = await this.vitenode.resolveId('vitest')
const vitestDir = projectVitestPath ? resolve(projectVitestPath.id, '../..') : rootDir
this.distPath = join(vitestDir, 'dist')

const node = this.vitenode
this.runner = new ViteNodeRunner({
root: server.config.root,
Expand Down Expand Up @@ -386,6 +381,11 @@ export class Vitest {
async start(filters?: string[]) {
this._onClose = []

// if Vitest is running globally, then we should still import local vitest if possible
const projectVitestPath = await this.vitenode.resolveId('vitest')
const vitestDir = projectVitestPath ? resolve(projectVitestPath.id, '../..') : rootDir
Copy link
Member

Choose a reason for hiding this comment

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

vs code extension doesn't call start at all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, really...
I had createRequire approach before 1037511. Would this work better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or we could delay it further to ctx.runFiles. Let me think...

Copy link
Member

Choose a reason for hiding this comment

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

The createRequire approach is wrong because we need Vite to process it. Having it in runFiles is fine. We can have a guard to not check this path if it is resolved, and reset it in configureServer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we need Vite's resolution? If the purpose is only to find local Vitest, would createRequire with require.resolve('vitest', { paths: [config.root] }) do the same?

This was the approach used for package installer:

if (process.versions.pnp) {
const targetRequire = createRequire(__dirname)
try {
targetRequire.resolve(dependency, { paths: [root, __dirname] })

Copy link
Member

Choose a reason for hiding this comment

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

We need Vite's resolution because files importing vitest are processed by Vite. It's possible that Vitest is called globally and resolved id is not correct, or there is an alias or resolveId interception

this.distPath = join(vitestDir, 'dist')

try {
await this.initCoverageProvider()
await this.coverageProvider?.clean(this.config.coverage.clean)
Expand Down
5 changes: 5 additions & 0 deletions test/cli/fixtures/plugin/basic.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import { expect, test } from "vitest";

test("basic", () => {
expect(1).toBe(1);
})
41 changes: 41 additions & 0 deletions test/cli/fixtures/plugin/vitest.config.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import { defineConfig } from 'vitest/config'

export default defineConfig({
plugins: [
{
name: "test-default",
configureServer() {
console.log("##test## configureServer(default)")
hi-ogawa marked this conversation as resolved.
Show resolved Hide resolved
},
buildStart() {
console.log("##test## buildStart(default)")
},
resolveId(source) {
console.log("##test## resolveId(default)")
console.log({ source })
},
transform(_code, id) {
console.log("##test## transform(default)")
console.log({ id })
},
},
{
name: "test-pre",
enforce: "pre",
configureServer() {
console.log("##test## configureServer(pre)")
},
buildStart() {
console.log("##test## buildStart(pre)")
},
resolveId(source) {
console.log("##test## resolveId(pre)")
console.log({ source })
},
transform(_code, id) {
console.log("##test## transform(pre)")
console.log({ id })
},
}
]
})
28 changes: 28 additions & 0 deletions test/cli/test/plugin.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import { Console } from 'node:console'
import { Writable } from 'node:stream'
import { expect, it, vi } from 'vitest'
import { runVitest } from '../../test-utils'

it('plugin hooks', async () => {
// capture console on main process
let stdout = ''
vi.stubGlobal('console', new Console({
stdout: new Writable({
write: (data, _, callback) => {
stdout += String(data)
callback()
},
}),
}))
await runVitest({ root: './fixtures/plugin' })
vi.unstubAllGlobals()

const lines = stdout.split('\n').filter(line => line.startsWith('##test##'))
expect(lines.slice(0, 5)).toEqual([
'##test## configureServer(pre)',
'##test## configureServer(default)',
'##test## buildStart(pre)',
'##test## buildStart(default)',
'##test## resolveId(pre)',
])
})
Loading