workers-sdk icon indicating copy to clipboard operation
workers-sdk copied to clipboard

fix: some peer dependency warnings

Open threepointone opened this issue 1 year ago • 7 comments

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

  • Tests
    • [ ] TODO (before merge)
    • [ ] Included
    • [x] Not necessary because: no functional changes
  • E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
    • [ ] I don't know
    • [ ] Required / Maybe required
    • [x] Not required because: no functional changes
  • Changeset (Changeset guidelines)
    • [ ] TODO (before merge)
    • [x] Included
    • [ ] Not necessary because:
  • Public documentation
    • [ ] TODO (before merge)
    • [ ] Cloudflare docs PR(s):
    • [x] Not necessary because: no api/behaviour changes

threepointone avatar Jun 25 '24 13:06 threepointone

🦋 Changeset detected

Latest commit: f785bc6db081969370767090f42fd8691ea0173a

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

changeset-bot[bot] avatar Jun 25 '24 13:06 changeset-bot[bot]

@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 avatar Jun 25 '24 14:06 threepointone

@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

dario-piotrowicz avatar Jun 26 '24 10:06 dario-piotrowicz

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 [email protected]: 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 avatar Jun 26 '24 12:06 andyjessop

@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 avatar Jun 26 '24 15:06 andyjessop

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

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

andyjessop avatar Jun 26 '24 15:06 andyjessop

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

threepointone avatar Jun 28 '24 10:06 threepointone