cloudflare / workers-sdk

⛅️ Home to Wrangler, the CLI for Cloudflare Workers®
https://developers.cloudflare.com/workers/
Apache License 2.0
2.41k stars 590 forks source link

fix: some peer dependency warnings #6144

Open threepointone opened 5 days ago

threepointone commented 5 days ago

This fixes some peer dependency warning we were seeing when doing installs/builds

changeset-bot[bot] commented 5 days ago

🦋 Changeset detected

Latest commit: 081a90c732a0596e8f6a8847b881966b99f3e4f9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package | Name | Type | | ------------------------------- | ----- | | @cloudflare/vitest-pool-workers | Patch |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

threepointone commented 5 days ago

@dario-piotrowicz are you familiar with the vitest-pool-workers codebase? I see the type errors here which seem legit, but I'm not sure what the fix should be. Is this something you can help with? No rush at all on this, or lemme know if someone else can help on it. If not, I'll be happy to dive in myself.

dario-piotrowicz commented 4 days ago

@dario-piotrowicz are you familiar with the vitest-pool-workers codebase? I see the type errors here which seem legit, but I'm not sure what the fix should be. Is this something you can help with? No rush at all on this, or lemme know if someone else can help on it. If not, I'll be happy to dive in myself.

@threepointone no sorry I am not really familiar with the vitest-pool-workers codebase, I am still totally happy to give it a look when I find the time if you want 🙂 (sometime next week I think)

also @petebacondarwin or @penalosa could be familiar with it

andyjessop commented 4 days ago

Vitest Pool Workers relies on a specific version of Vitest, which I think could cause issues such as this. When you pnpm install on this branch, you get:

 WARN  Issues with peer dependencies found
.
└─┬ @vitest/ui 1.6.0
  └── ✕ unmet peer vitest@1.6.0: found 1.2.2

I believe the actual issue is to do with a conflict in the way Vitest extends the vite types. It adds this ambient declaration:

declare module 'vite' {
    interface UserConfig {
        /**
         * Options for Vitest
         */
        test?: VitestInlineConfig;
    }
}

And for some reason, it's not picking this up, and is instead using the UserConfig from vite, which doesn't contain this test property.

andyjessop commented 4 days ago

@threepointone @petebacondarwin I think there is a solution to this, but I don't believe it's a long term solution.

This line is causing the Vite version to resolve to 5.3.1 in the root. And this version of vite is causing this type mismatch in my comment above.

The solution I have is to lock the vite version in vitest-pool-workers to 5.1.0:

"vite": "5.1.0",

(Note that in order to update this correctly, you will have to first checkout the pnpm-lock.yaml file from main and then pnpm install again, because there is some apparent hysteresis in the pnpm-lock.yaml, which means that the vitest-pool-workers package will still see the 5.3.1 version if you only pnpm install).

The other solution I would think is to upgrade Vitest in vitest-pool-workers, but I read that the version there is locked.

andyjessop commented 4 days ago

Referencing this here, because it's a different fix to the same issue:

https://github.com/cloudflare/next-on-pages/pull/811#discussion_r1652657818

threepointone commented 2 days ago

@andyjessop feel free to take a call and own this PR?