fix: Yarn 3/4 PnP compatibility

[ianlet]

What is it?

• Feature / enhancement

Description

As documented in #5038, we cannot start a Qwik app in a Yarn 3 or 4 workspace with PnP because some dependencies are not properly declared in both qwik and qwik-city.

The changes in this PR are simply to make the dependencies explicit or to make them as an acutal dependency instead of just a dev-dependency.

Close #5038.

For reference: here are the places I found in the code requiring each dependency.

• Qwik
  • Vite
    • packages/qwik/src/optimizer/src/plugins/eslint-plugin.ts
    • packages/qwik/src/optimizer/src/plugins/vite-server.ts
    • packages/qwik/src/optimizer/src/plugins/vite-utils.ts
    • packages/qwik/src/optimizer/src/plugins/image-size-server.ts
    • packages/qwik/src/optimizer/src/plugins/rollup.ts
    • packages/qwik/src/optimizer/src/plugins/rollup.unit.ts
    • packages/qwik/src/optimizer/src/plugins/plugin.ts
    • packages/qwik/src/optimizer/src/plugins/vite.ts
    • packages/qwik/src/optimizer/src/plugins/vite.unit.ts
• Qwik City
  • Vite
    • packages/qwik-city/buildtime/vite/image-jsx.ts
    • packages/qwik-city/adapters/cloud-run/vite/index.ts
    • packages/qwik-city/buildtime/vite/config.ts
    • packages/qwik-city/buildtime/vite/plugin.ts
    • packages/qwik-city/buildtime/vite/dev-server.ts
    • packages/qwik-city/adapters/shared/vite/index.ts
    • packages/qwik-city/static/main-thread.ts
    • packages/qwik-city/runtime/vite.config.ts
  • Undici
    • packages/qwik-city/middleware/node/node-fetch.ts
    • packages/qwik-city/runtime/src/service-worker/utils.unit.ts

Use cases and why

Yarn enforces a more strict approach than other package managers (you can read more about it here) regarding how dependencies are declared. In order to improve maintainability and prevent future issues, it forces all required dependency to be explicitly declared.

Even though Qwik is managed with pnpm and it wasn't reported as an issue until now, being aware of this and declaring dependencies explicitly should improve both the code maintainability and the adoption of Qwik (for those using Yarn 3 or 4 with PnP not in loose mode).

Checklist:

• My code follows the developer guidelines of this project
• I have performed a self-review of my own code

[stackblitz]

Run & review this pull request in StackBlitz Codeflow.

[netlify]

👷 Deploy request for qwik-insights pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	efd94be

[gioboa]

Thanks for your PR and for the interesting description. So with only these small changes yarn 3/4 works correctly, right?

[ianlet]

@gioboa I was able to run a newly created qwik app (with yarn create qwik) in both Yarn 3 & 4 with PnP. I tried to make a few changes here and there and I didn't notice any new issues.

So as far as I know, it now works correctly. But it doesn't mean that it covers every cases and that something else could come up later on.

Something that could be interesting is to automate a test suite that tries to start a qwik app in a Yarn workspace with PnP. That way we could know in the future if a new require was added without properly declaring its dependency. What do you think?

[gioboa]

I think it look so good, well done 👍 let's wait the core team review

[gioboa]

this PR is approved by a core team member so @ianlet pls solve the conflicts and we will merge it.

[ianlet]

@gioboa PR updated. FYI the "auto-merge" was automatically disabled when I pushed.

[ianlet]

one of the checks is failing, complaining about an additional dependency

1. is it still a valuable check to keep?
2. should I adapt the script to make it pass?

[gioboa]

Yep, we need to adapt the script to allow the extra deps we introduced

[zanettin]

Hi @ianlet 👋
Sry, i've reverted a bit too much on your PR 🙈 but at least the CI is happy now.
do you want to give it a final try before we ship it?
thanks again for your help on this 🙏

[ianlet]

@zanettin thanks for the update! Per se it seems to be working.

Though I made some more tests with my production app (which is in a Yarn 4 workspace) and now the following error is completely preventing my app to run:

@builder.io/qwik-city tried to access @qwik-city-plan/package.json, but it isn't declared in its dependencies; this makes the require call ambiguous and unsound.

I tried simply upgrading my app from 1.2.12 to 1.2.15 and the error is here as well.

So even though this PR improves the Yarn 3 & 4 compatibility, it doesn't solve it as new changes in 1.2.13, 1.2.14 or 1.2.15 are now completely breaking the build (before it was only warnings). Meaning this PR will not fix #5038 and needs more work to get rid of all warnings/problems.

I don't mind tracking down every dependency problem I can find, but I'm afraid that it will not prevent errors to come up again in the future. Especially as Yarn is not the tool used for this repo.

Do you want me to work on a new check in the CI to ensure it keeps working for a Yarn app using Qwik? That way we can know if new changes break something.