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: work-around for pnpm catalog support #32

Merged
merged 1 commit into from
Mar 10, 2025

Conversation

AriPerkkio
Copy link
Member

@AriPerkkio AriPerkkio commented Mar 9, 2025

Copy link

stackblitz bot commented Mar 9, 2025

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@AriPerkkio AriPerkkio marked this pull request as draft March 9, 2025 08:40
@AriPerkkio AriPerkkio force-pushed the fix/pnpm-catalog-support branch from 9dd8ecd to 118ba7a Compare March 9, 2025 08:47
@AriPerkkio
Copy link
Member Author

Using link: instead of file: almost works. Only browser mode fails, as it's unable to load other linked packages. This happens locally too when using linked packages. 🤷‍♂️

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Mar 9, 2025

How about using packed .tgz as override with pnpm pack? for example, overrides: { vitest: "/xxx/packages/vitest/vitest.tar.tgz" }. If it complicates things, I'm fine with a simple revert though.

@AriPerkkio
Copy link
Member Author

At least locally that seems to work. Though there are some peer dep warnings, but it should be fine.

 WARN  Issues with peer dependencies found
.
├─┬ vitest 3.0.8
│ ├── ✕ unmet peer @vitest/browser@file:/Users/x/vitest-ecosystem-ci/workspace/vitest/packages/browser/vitest-browser-3.0.8.tgz: found 3.0.8
│ └── ✕ unmet peer @vitest/ui@file:/Users/x/vitest-ecosystem-ci/workspace/vitest/packages/ui/vitest-ui-3.0.8.tg

@AriPerkkio AriPerkkio force-pushed the fix/pnpm-catalog-support branch from 118ba7a to ba48f6c Compare March 9, 2025 18:18
@AriPerkkio AriPerkkio marked this pull request as ready for review March 9, 2025 18:21
@@ -350,9 +350,12 @@ export async function buildVitest({ verify = false }) {
cd(vitestPath)
const frozenInstall = getCommand('pnpm', 'frozen')
const runBuild = getCommand('pnpm', 'run', ['build'])
const runPack = `pnpm -r --filter=./packages/** exec -- pnpm pack`
Copy link
Member Author

Choose a reason for hiding this comment

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

The getCommand from @antfu/ni didn't support this so hardcoding the command here.

@AriPerkkio AriPerkkio requested a review from hi-ogawa March 9, 2025 18:23
@gluxon
Copy link

gluxon commented Mar 9, 2025

Agree with @hi-ogawa's suggestion to use pnpm pack! 🥳

Here's my explanation before I saw @hi-ogawa's message.

Running a pnpm pack to test vitest in other repositories locally is good for other reasons. You get a portable .tgz file that's closer to what would be published to the NPM registry. Linking in the source directory of the vitest would cause a bunch of other side-effects I suspect aren't desirable.

@gluxon gluxon mentioned this pull request Mar 9, 2025
18 tasks
@AriPerkkio
Copy link
Member Author

Using pnpm pack here indeed fixed the issue. I first tried using link: instead of file:, but that has some weird monorepo package resolving issues in Vitest's browser imports.

With pnpm pack we end up with following changes in tested projects' package.json's devDependencies and pnpm.overrides:

{
-  vitest: 'file:/Users/x/vitest-ecosystem-ci/workspace/vitest/packages/vitest',
-  'vite-node': 'file:/Users/x/vitest-ecosystem-ci/workspace/vitest/packages/vite-node',
-  '@vitest/browser': 'file:/Users/x/vitest-ecosystem-ci/workspace/vitest/packages/browser',
-  '@vitest/coverage-istanbul': 'file:/Users/x/vitest-ecosystem-ci/workspace/vitest/packages/coverage-istanbul',
-  '@vitest/expect': 'file:/Users/x/vitest-ecosystem-ci/workspace/vitest/packages/expect',
-  '@vitest/snapshot': 'file:/Users/x/vitest-ecosystem-ci/workspace/vitest/packages/snapshot',
-  '@vitest/ui': 'file:/Users/x/vitest-ecosystem-ci/workspace/vitest/packages/ui',
-  '@vitest/web-worker': 'file:/Users/x/vitest-ecosystem-ci/workspace/vitest/packages/web-worker',
-  '@vitest/coverage-v8': 'file:/Users/x/vitest-ecosystem-ci/workspace/vitest/packages/coverage-v8',
-  '@vitest/runner': 'file:/Users/x/vitest-ecosystem-ci/workspace/vitest/packages/runner',
-  '@vitest/spy': 'file:/Users/x/vitest-ecosystem-ci/workspace/vitest/packages/spy',
-  '@vitest/utils': 'file:/Users/x/vitest-ecosystem-ci/workspace/vitest/packages/utils',
-  '@vitest/ws-client': 'file:/Users/x/vitest-ecosystem-ci/workspace/vitest/packages/ws-client',
-  '@vitest/pretty-format': 'file:/Users/x/vitest-ecosystem-ci/workspace/vitest/packages/pretty-format',
-  '@vitest/mocker': 'file:/Users/x/vitest-ecosystem-ci/workspace/vitest/packages/mocker'
+  vitest: 'file:/Users/x/vitest-ecosystem-ci/workspace/vitest/packages/vitest/vitest-3.0.8.tgz',
+  'vite-node': 'file:/Users/x/vitest-ecosystem-ci/workspace/vitest/packages/vite-node/vite-node-3.0.8.tgz',
+  '@vitest/browser': 'file:/Users/x/vitest-ecosystem-ci/workspace/vitest/packages/browser/vitest-browser-3.0.8.tgz',
+  '@vitest/coverage-istanbul': 'file:/Users/x/vitest-ecosystem-ci/workspace/vitest/packages/coverage-istanbul/vitest-coverage-istanbul-3.0.8.tgz',
+  '@vitest/expect': 'file:/Users/x/vitest-ecosystem-ci/workspace/vitest/packages/expect/vitest-expect-3.0.8.tgz',
+  '@vitest/snapshot': 'file:/Users/x/vitest-ecosystem-ci/workspace/vitest/packages/snapshot/vitest-snapshot-3.0.8.tgz',
+  '@vitest/ui': 'file:/Users/x/vitest-ecosystem-ci/workspace/vitest/packages/ui/vitest-ui-3.0.8.tgz',
+  '@vitest/web-worker': 'file:/Users/x/vitest-ecosystem-ci/workspace/vitest/packages/web-worker/vitest-web-worker-3.0.8.tgz',
+  '@vitest/coverage-v8': 'file:/Users/x/vitest-ecosystem-ci/workspace/vitest/packages/coverage-v8/vitest-coverage-v8-3.0.8.tgz',
+  '@vitest/runner': 'file:/Users/x/vitest-ecosystem-ci/workspace/vitest/packages/runner/vitest-runner-3.0.8.tgz',
+  '@vitest/spy': 'file:/Users/x/vitest-ecosystem-ci/workspace/vitest/packages/spy/vitest-spy-3.0.8.tgz',
+  '@vitest/utils': 'file:/Users/x/vitest-ecosystem-ci/workspace/vitest/packages/utils/vitest-utils-3.0.8.tgz',
+  '@vitest/ws-client': 'file:/Users/x/vitest-ecosystem-ci/workspace/vitest/packages/ws-client/vitest-ws-client-3.0.8.tgz',
+  '@vitest/pretty-format': 'file:/Users/x/vitest-ecosystem-ci/workspace/vitest/packages/pretty-format/vitest-pretty-format-3.0.8.tgz',
+  '@vitest/mocker': 'file:/Users/x/vitest-ecosystem-ci/workspace/vitest/packages/mocker/vitest-mocker-3.0.8.tgz'
}

@gluxon
Copy link

gluxon commented Mar 9, 2025

Using pnpm pack here indeed fixed the issue. I first tried using link: instead of file:, but that has some weird monorepo package resolving issues in Vitest's browser imports.

Gotcha. The browser issues aren't too surprising when using link:. The link: protocol simply creates a symlink to the target repository, which then forces anything that performs node_modules resolution to use the node_modules directory of the other monorepo, rather than node_modules of the current repo. It basically "jumps" and uses the other repo's dependencies.

I think you're trying to simulate what would happen if someone depended on vitest packages that would be downloaded through the NPM registry. The link: protocol does something very different than that. I'm glad everyone here ended up with using the file: protocol with pnpm pack instead.

Comment on lines 494 to +500
if (pm === 'pnpm') {
const version = parseVitestVersion(options.vitestPath)

for (const key in overrides) {
const tar = key.replace('@', '').replace('/', '-')
overrides[key] = `${overrides[key]}/${tar}-${version}.tgz`
}
Copy link
Contributor

Choose a reason for hiding this comment

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

How about other package managers? I'd assume they should also break when seeing catalog:, but it's probably not tested right now?

Copy link

Choose a reason for hiding this comment

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

Right. Other packages managers would break when seeing catalog:. I'd note that's not unique to the catalog: protocol or pnpm. The file:, link:, and workspace: protocols are also meant to be "local-only" and don't work across different repos. There's a requirement for a "portability" step (e.g. publish or pack) to remove protocols like workspace: and catalog:.

pnpm itself throws when encountering the catalog: protocol in NPM packages since we don't want to encourage non-portable specifiers being published to the NPM registry.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess the test setup doesn't contain other package managers. Otherwise we should already have run into issues with workspace: protocol. 🤔

I think proper solution would be to use released packages from https://pkg.pr.new - I think Vite already has that kind of workflow.

Copy link
Contributor

@hi-ogawa hi-ogawa left a comment

Choose a reason for hiding this comment

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

Approving this to unblock CI 👍
I think I prefer this than reverting catalog entirely. Vite integrated https://pkg.pr.new, but I'm seeing quite a bit of flakiness there due to package download failure, so I'm not sure we should go setting up the same right now.

@AriPerkkio AriPerkkio merged commit dd127ba into vitest-dev:main Mar 10, 2025
1 check passed
@AriPerkkio AriPerkkio deleted the fix/pnpm-catalog-support branch March 10, 2025 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants