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

Migrate away from Yarn classic (v1) #1837

Closed
kleinfreund opened this issue Nov 28, 2023 · 4 comments · Fixed by #2402
Closed

Migrate away from Yarn classic (v1) #1837

kleinfreund opened this issue Nov 28, 2023 · 4 comments · Fixed by #2402
Labels
kind/support Support issue triage/accepted The issue was reviewed and is complete enough to start working on it
Milestone

Comments

@kleinfreund
Copy link
Contributor

kleinfreund commented Nov 28, 2023

Description

Yarn classic (v1) hasn’t been a recent version of Yarn for a while and is no longer maintained (the last change was in September ’22)

We should probably migrate away from it.

Options are Yarn, pnpm, and npm. See below comments for my evaluation:

TL;DR verdict: npm > pnpm > yarn

@kleinfreund kleinfreund added triage/pending This issue will be looked at on the next triage meeting kind/support Support issue labels Nov 28, 2023
@jakubdyszkiewicz jakubdyszkiewicz added triage/accepted The issue was reviewed and is complete enough to start working on it and removed triage/pending This issue will be looked at on the next triage meeting labels Dec 4, 2023
@lahabana lahabana added this to the backlog milestone Jan 9, 2024
@kleinfreund
Copy link
Contributor Author

kleinfreund commented Jan 18, 2024

Evaluating yarn (v4)

  • Con: Requires us to come up with some workaround for the workspace scripts issue (details below) that seems, for now, more involved than it would with pnpm

Scripts in workspace don’t find binaries from dev dependencies

In short, Yarn versions >1 don’t link binaries from the root of a workspace (see relevant issue: yarnpkg/yarn#4543).

This breaks our workspace-level and project-level scripts with the command not found errors. For example, running yarn lint:km complains about a missing eslint command. This is because only packages/kuma-gui has eslint as a dev dependency and so eslint only exists in packages/kuma-gui/node_modules.

On the workspace level, one could work-around this limitation by always listing all dev dependencies also as dev dependencies of the root package.json. Then, one can run, for example, yarn lint:km just fine. However, one still can’t run yarn lint from packages/kong-mesh-gui because that project itself doesn’t list eslint (again, just an example) as a dev dependency and therefore its node_modules directory doesn’t in fact contain eslint. To work-around that limitation, one would also have to install all dev dependencies in all projects. That’s just nonsense. Alternatively, one defines all scripts in the root level. Neither options sound great.

Pnpm has the same issue, but there is a solution.

@kleinfreund
Copy link
Contributor Author

kleinfreund commented Apr 3, 2024

Evaluating pnpm (v8)

  • Pro: Minimizes lock file entry duplication on install
  • Con: Requires us to configure shamefully-hoist=true or node-linker=hoisted to (1) avoid unexpected TypeScript errors when running vue-tsc and (2) run workspace scripts in kong-mesh-gui
  • Neutral: CI workflows require manual installation of pnpm (in some way an advantage because we control it more precisely; in another way a disadvantage because we have to deal with it)

Unexpected TypeScript errors

On migrating the project to pnpm, the lint:ts script starts failing with unexpected (and incorrect) TypeScript errors:

Output
> kuma-gui@2.6.0 lint:ts /home/phil/kong/kuma-gui
> vue-tsc --noEmit

src/app/application/index.ts:34:16 - error TS2664: Invalid module name in augmentation, module '@vue/runtime-core' cannot be found.

34 declare module '@vue/runtime-core' {
                  ~~~~~~~~~~~~~~~~~~~

src/app/application/services/i18n/I18n.ts:7:16 - error TS2664: Invalid module name in augmentation, module 'intl-messageformat' cannot be found.

7 declare module 'intl-messageformat' {
                 ~~~~~~~~~~~~~~~~~~~~

src/app/application/services/i18n/I18n.ts:64:26 - error TS2339: Property 'defaultMessage' does not exist on type 'Options'.

64             if (rest[2]?.defaultMessage) {

# ... abridged ...

Found 23 errors in 14 files.

Errors  Files
     1  src/app/application/index.ts:34
     3  src/app/application/services/i18n/I18n.ts:7
     1  src/app/data-planes/components/data-plane-traffic/ServiceTrafficCard.vue:12
     2  src/app/data-planes/views/DataPlaneConfigView.vue:49
     1  src/app/data-planes/views/DataPlaneDetailView.vue:373
     1  src/app/data-planes/views/DataPlaneInboundSummaryView.vue:43
     1  src/app/data-planes/views/DataPlaneOutboundSummaryView.vue:30
     2  src/app/meshes/views/MeshDetailView.vue:47
     1  src/app/policies/components/PolicyList.vue:46
     2  src/app/policies/views/PolicyDetailView.vue:129
     2  src/app/policies/views/PolicySummaryView.vue:95
     2  src/app/services/views/ServiceConfigView.vue:59
     2  src/app/zone-egresses/views/ZoneEgressConfigView.vue:48
     2  src/app/zone-ingresses/views/ZoneIngressConfigView.vue:48
 ELIFECYCLE  Command failed with exit code 2.

The issue goes away when installing dependencies with shamefully-hoist=true or node-linker=hoisted being configured.

Workspace scripts

By default, pnpm behaves like Yarn in that binaries that come from one workspace package are only available in that package and not sym-linked to the root.

In pnpm, this can be worked around by setting shamefully-hoist=true or node-linker=hoisted. Yarn has the same issue, but no workaround.

@kleinfreund
Copy link
Contributor Author

kleinfreund commented Apr 3, 2024

Evaluating npm (v10)

  • Pro: It just works.
  • Con (possible long-term issue): Can’t npm install if there are peer dependency conflicts
  • Con (minor): passing arguments to package scripts requires the -- to delimit the package script from the arguments (e.g. npm run test:browser -- --spec ...).

Issue: peer dependency conflicts block install (resolved for the time being)

One can’t run npm install if there is a peer dependency conflict. For example, we have a dependency on @whyframe/vue. When I initially tried using npm, this package had a peer dependency requirement of Vite 3 or 4 and the command failed with the below error as we depend on Vite 5.

npm ERR! code ERESOLVE
npm ERR! ERESOLVE unable to resolve dependency tree
npm ERR! 
npm ERR! While resolving: kuma-gui@2.6.0
npm ERR! Found: vite@5.1.1
npm ERR! node_modules/vite
npm ERR!   dev vite@"5.1.1" from the root project
npm ERR! 
npm ERR! Could not resolve dependency:
npm ERR! peer vite@"^3.0.0 || ^4.0.0" from @whyframe/vue@0.1.6
npm ERR! node_modules/@whyframe/vue
npm ERR!   dev @whyframe/vue@"0.1.6" from the root project
npm ERR! 
npm ERR! Fix the upstream dependency conflict, or retry
npm ERR! this command with --force or --legacy-peer-deps
npm ERR! to accept an incorrect (and potentially broken) dependency resolution.
npm ERR! 
npm ERR! 
npm ERR! For a full report see:
npm ERR! /home/phil/.npm/_logs/2024-02-13T10_37_00_901Z-eresolve-report.txt

npm ERR! A complete log of this run can be found in: /home/phil/.npm/_logs/2024-02-13T10_37_00_901Z-debug-0.log

@kleinfreund
Copy link
Contributor Author

We decided to go with npm.

@kleinfreund kleinfreund self-assigned this Apr 15, 2024
@kleinfreund kleinfreund modified the milestones: backlog, 2.8.x Apr 15, 2024
@kleinfreund kleinfreund removed their assignment Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/support Support issue triage/accepted The issue was reviewed and is complete enough to start working on it
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants