Skip to content

fix(vm): fix external module resolve error with deps optimizer query for encoded URI#10658

Merged
sheremet-va merged 1 commit into
vitest-dev:mainfrom
SveLil:main
Jun 25, 2026
Merged

fix(vm): fix external module resolve error with deps optimizer query for encoded URI#10658
sheremet-va merged 1 commit into
vitest-dev:mainfrom
SveLil:main

Conversation

@SveLil

@SveLil SveLil commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Description

Resolves #10657

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. If the feature is substantial or introduces breaking changes without a discussion, PR might be closed.
  • Ideally, include a test that fails without this PR but passes with it.
  • Please, don't make changes to pnpm-lock.yaml unless you introduce a new test example.
  • Please check Allow edits by maintainers to make review process faster. Note that this option is not available for repositories that are owned by Github organizations.

Tests

  • Run the tests with pnpm test:ci

Changesets

  • Changes in changelog are generated from PR name. Please, make sure that it explains your changes in an understandable manner. Please, prefix changeset messages with feat:, fix:, perf:, docs:, or chore:.

@SveLil

SveLil commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

Note: This is only a fix for ONE cleanUrl, there are quite a few, maybe time for a cleanup?

@sheremet-va

Copy link
Copy Markdown
Member

Why does vm receive an encoded string when cleaning the URL? Is it passed down incorrectly?

@hi-ogawa hi-ogawa left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The only source of glitch seems like the single point here in getCachedVitestImport

so fix belongs there and not the general cleanUrl side.


Unfortunately, we get ?v=xxx only when vitest is inside node_modules so that seems like the reason we don't repro in our e2e.

@SveLil

SveLil commented Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

I just debugged, for me it's a few lines later:

const externalize = pathToFileURL(path).toString()

I'll push the fix soon.

@netlify

netlify Bot commented Jun 25, 2026

Copy link
Copy Markdown

Deploy Preview for vitest-dev ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 2aa1237
🔍 Latest deploy log https://app.netlify.com/projects/vitest-dev/deploys/6a3cd26ae875be00085aa325
😎 Deploy Preview https://deploy-preview-10658--vitest-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

const externalize = id.startsWith('file://')
? id
: pathToFileURL(id).toString()
: decodeURIComponent(pathToFileURL(id).toString())

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

From what I can guess, the idea isn't decoding. The idea is to have pathToFileURL to not include ?param by splitting id so it aligns with the fix on consumer side #10024.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Perhaps something like this here and below.

const { file, postfix } = splitFileAndPostfix(id)
const externalize = id.startsWith('file://') ? id : `${pathToFileURL(file)}${postfix}`

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No problem at all, that works, too

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Pushed a version that will include the param but returns a non-encoded URI

@pkg-pr-new

pkg-pr-new Bot commented Jun 25, 2026

Copy link
Copy Markdown
@vitest/browser

npm i https://pkg.pr.new/@vitest/browser@630c027

@vitest/browser-playwright

npm i https://pkg.pr.new/@vitest/browser-playwright@630c027

@vitest/browser-preview

npm i https://pkg.pr.new/@vitest/browser-preview@630c027

@vitest/browser-webdriverio

npm i https://pkg.pr.new/@vitest/browser-webdriverio@630c027

@vitest/coverage-istanbul

npm i https://pkg.pr.new/@vitest/coverage-istanbul@630c027

@vitest/coverage-v8

npm i https://pkg.pr.new/@vitest/coverage-v8@630c027

@vitest/expect

npm i https://pkg.pr.new/@vitest/expect@630c027

@vitest/mocker

npm i https://pkg.pr.new/@vitest/mocker@630c027

@vitest/pretty-format

npm i https://pkg.pr.new/@vitest/pretty-format@630c027

@vitest/snapshot

npm i https://pkg.pr.new/@vitest/snapshot@630c027

@vitest/spy

npm i https://pkg.pr.new/@vitest/spy@630c027

@vitest/ui

npm i https://pkg.pr.new/@vitest/ui@630c027

@vitest/utils

npm i https://pkg.pr.new/@vitest/utils@630c027

vitest

npm i https://pkg.pr.new/vitest@630c027

@vitest/web-worker

npm i https://pkg.pr.new/@vitest/web-worker@630c027

commit: 630c027

@hi-ogawa hi-ogawa left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you!
I verified the repro with pkg.pr.new. Not being able to e2e is unfortunate but can be revisited in the future.

@hi-ogawa hi-ogawa requested a review from sheremet-va June 25, 2026 09:27
@hi-ogawa

Copy link
Copy Markdown
Collaborator

This means vm + optimizer has been broken entire v4 phase. We may want to backport this to v4.

@SveLil

SveLil commented Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

This means vm + optimizer has been broken entire v4 phase. We may want to backport this to v4.

Yes, I'm on v4 and I will backport it

SveLil added a commit to SveLil/vitest that referenced this pull request Jun 25, 2026
@sheremet-va sheremet-va merged commit 90c4ed4 into vitest-dev:main Jun 25, 2026
19 checks passed
@sheremet-va

Copy link
Copy Markdown
Member

Please, backport to v4 branch

@SveLil

SveLil commented Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

See #10661 10661

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

mThreads/vmForks pool crashes with ERR_MODULE_NOT_FOUND when deps.optimizer.client.enabled is true

3 participants