Migrate Project from npm to Yarn #5106

rak-phillip · 2023-07-05T20:51:49Z

This pull request aims to migrate our project from npm to yarn for improved consistency and compatibility with Rancher Dashboard and Rancher Shell.

This migration also allows for a better debugging and development experience when developing plugins for @rancher/shell via yarn link. yarn link has proven to be more reliable than npm link, making debugging @rancher/shell a lot more easier.

🗒️ Notes

Before working with this PR, make sure that you have yarn installed globally. This should be done via npm install --global yarn. See the yarn docs¹ for more information.

We are sticking to the "classic" version of Yarn for parity with Rancher Dashboard. We may want to consider upgrading to Yarn 2+ in the future². Doing this will resolve at least one bug that I encountered with Windows CI³.

Ultimately, we should anticipate Rancher Desktop development to remain the same as it does today, albeit running commands with yarn instead of npm.

I encountered an error in Windows 11 that looked like this

2023/07/10 14:13:37 building a plan for generation
2023/07/10 14:13:37 generation target ./
2023/07/10 14:13:37 CreateFile go: The system cannot find the file specified.
exit status 1
pkg\dockerproxy\generate.go:24: running "go": exit status 1
  inflating: C:/Users/phillip/Development/rancher-desktop/resources/host/wix/torch.exe
  inflating: C:/Users/phillip/Development/rancher-desktop/resources/host/wix/torch.exe.config
POSTINSTALL ERROR:  SpawnError: go exited with code 1
    at ChildProcess.<anonymous> (C:\Users\phillip\Development\rancher-desktop\pkg\rancher-desktop\utils\childProcess.ts:289:14)
    at ChildProcess.emit (node:events:513:28)
    at ChildProcess.emit (node:domain:489:12)
    at Process.ChildProcess._handle.onexit (node:internal/child_process:293:12) {
  command: [ 'go', 'generate', '-x', 'pkg/dockerproxy/generate.go' ],
  code: 1,
  [Symbol(child-process.command)]: 'go generate -x pkg/dockerproxy/generate.go'
}

To resolve this issue, I had to update go to version 1.20.5. I'm still unsure why this would fail on 1.20.3 and I'm interested if anybody has any feedback as to why.

Changes Made

Resolved build warnings and errors that arose as a result of the change in tooling.
Updated developer documentation to reflect the migration to Yarn.
Updated CI/CD steps to incorporate Yarn commands.

closes #3723

rak-phillip · 2023-07-06T22:36:52Z

I requested 3 reviewers to get good coverage on this change. It would be best if we were able to get this through without any noted regressions in our supported development platforms. Let me know if you have any questions, comments, or concerns.

ericpromislow · 2023-07-07T00:22:36Z

Not ready to do the big review button yet.

This looks straightforward, but I definitely can't be the only reviewer here. Some notes:

on first run, yarn dev ran as a blank window for main and prefs, but the dashboard was populated. I shut it down, reran, and it was fine.

I need to test this on Windows as well, in a VM. I'm concerned about our setup scripts on Windows, and the readme.

The bats tests often refer to npm; some of these will need to be changed to yarn, like RD_LOCATION=yarn

We should have a goal that someone who later joins the project doesn't have to be aware that we once used npm as our main build tool, and won't be by looking at the current code.

ericpromislow

see my bigger comment...

adamkpickering

I don't see anything wrong with the current changes. I tested everything I can think of, and it all works. However:

README.md is missing instructions on how to install yarn for all three platforms. This is particularly important because we are using yarn classic, which is not obvious.
npm still appears in many places in the repo (bats tests and README.md are the most prominent ones). I think at least some of these cases should be replaced by yarn. I won't list them here - you can see them with git grep npm.
When switching to yarn, I had to do a yarn global add node-gyp to get yarn install to work. Maybe we should add this to the Linux developer docs? It's odd that I never had to do this with npm though.

rak-phillip · 2023-07-07T23:23:11Z

* README.md is missing instructions on how to install `yarn` for all three platforms. This is particularly important because we are using `yarn` classic, which is not obvious.

This is a good catch and will be easy to fix.

* `npm` still appears in many places in the repo (bats tests and README.md are the most prominent ones). I _think_ at least some of these cases should be replaced by `yarn`. I won't list them here - you can see them with `git grep npm`.

This is true, although not all instances of the npm will be invalid across the board. I do see quite a few instances under bats/ that we can clean up.

* When switching to `yarn`, I had to do a `yarn global add node-gyp` to get `yarn install` to work. Maybe we should add this to the Linux developer docs? It's odd that I never had to do this with `npm` though.

This is interesting and requires more testing. I'm working in Linux (openSUSE Tumbleweed) and have not needed to do the same. I think that we should investigate more closely to understand why you would need this requirement and I wouldn't.

rak-phillip · 2023-07-10T18:10:02Z

@adamkpickering I'm not able to repro the scenario you outlined above with the following repro steps

When switching to yarn, I had to do a yarn global add node-gyp to get yarn install to work.

Download an install openSUSE Leap 15.3 ¹ in a VM
Clone the Rancher Desktop repo and follow the install instructions ²
Confirm that npm run dev works
Checkout this PR
Install yarn ³
rm -rf node_modules
Install dependencies and run the project with yarn: yarn && yarn dev

This succeeds without any failure in my VM. I don't think that the installation of yarn should interfere with the original node-gyp installation step unless the npm/node version changed. Are you in a position where you can try again to reproduce this?

rak-phillip · 2023-07-10T20:12:59Z

@adamkpickering @ericpromislow I think this is ready for another review.

Since our last review, I've update the installation instructions to include yarn and reviewed the remaining usages of npm in the project.

I think the remaining instances of npm used throughout look appropriate. This is what I used to review

➤ grep -r npm --exclude-dir={node_modules, resources, dist, .git} --exclude="yarn.lock"

A majority of the matches for bats come from bats/bats-assert, bats/bats-core, and bats/bats-support; I don't believe it makes sense to update these areas. I'm more than willing to investigate more closely if we're able to identify issues with our bats usage as a result of this change.

bats/tests/helpers/defaults.bash

rak-phillip · 2023-07-11T19:55:40Z

@adamkpickering I made the changes to BATS, this is ready for another review when you are ready.

jandubois · 2023-07-11T20:00:59Z

The BATS changes look fine to me; I won't be reviewing the rest of the PR

ericpromislow

$ git grep -n using_npm_run_dev bats
bats/tests/containers/auto-start.bats:34:    if using_npm_run_dev; then
bats/tests/containers/auto-start.bats:60:    if using_npm_run_dev; then

These two instances need to be changed to using_dev_mode

I didn't run all of bats. yarn dev worked fine, trying build... package now.
I see that some of our identifiers/strings that contain the npm substring are coming
from node_modules/ and they're out of our control -- that's fine with me.

ericpromislow

Looks good. Let's do it.

rak-phillip · 2023-07-14T17:48:50Z

I will be waiting for #5162 to merge before this so that we can rebase and resolve the Windows issues described above.

rak-phillip · 2023-07-17T20:34:43Z

@adamkpickering @ericpromislow rebased and ready for one more review. Like @ericpromislow mentioned, there were a few more instances of using_npm_run_dev that needed to be replaced after rebasing. Everything else stays the same since the last review.

adamkpickering

Everything is working as far as I can tell.

Update lockfile Signed-off-by: Phillip Rak <rak.phillip@gmail.com>

This resolves `TS18022` that arose as a result of pinning typescript to `4.5.4` ``` ERROR ERROR in pkg/rancher-desktop/backend/backendHelper.ts:24:10 11:08:03 TS18022: A method cannot be named with a private identifier. 22 | * preceded by an odd number of backslashes. 23 | */ > 24 | static #escapeChar(match: any, slashes: string, char: string) { | ^^^^^^^^^^^ 25 | if (slashes.length % 2 === 0) { 26 | slashes += '\\'; 27 | } ``` Signed-off-by: Phillip Rak <rak.phillip@gmail.com>

Resolves TS7006: Parameter 'x' implicitly has an 'any' type ``` ERROR in pkg/rancher-desktop/components/MountTypeSelector.vue:95:15 TS7006: Parameter 'x' implicitly has an 'any' type. 93 | 94 | return items > 95 | .map((x) => { | ^ 96 | return { 97 | label: this.t(`virtualMachine.mount.type.options.9p.options.${ setting }.options.${ x.replace('.', '') }`), 98 | value: x, ``` Signed-off-by: Phillip Rak <rak.phillip@gmail.com>

``` ERROR in pkg/rancher-desktop/main/extensions/extensions.ts:292:11 TS2532: Object is possibly 'undefined'. 290 | await this.client.copyFile( 291 | this.image, > 292 | data.root, | ^^^^ 293 | path.join(uiDir, name), 294 | { namespace: this.extensionNamespace }); 295 | } catch (ex: any) { ``` Signed-off-by: Phillip Rak <rak.phillip@gmail.com>

Signed-off-by: Phillip Rak <rak.phillip@gmail.com>

Co-authored-by: Mark Yen <3977982+mook-as@users.noreply.github.com> Signed-off-by: Phillip Rak <rak.phillip@gmail.com>

Signed-off-by: Phillip Rak <rak.phillip@gmail.com>

Replace `npm ci` with `yarn install --frozen-lockfile` Install yarn globally for cirrus Package with python Remove yarn upgrade step change yarn registry Increase timeout duration Update yarn commands Signed-off-by: Phillip Rak <rak.phillip@gmail.com>

Signed-off-by: Phillip Rak <rak.phillip@gmail.com>

- BATS: Update `using_dev` to `using_dev_mode` - Replace remaining instances of `using_npm_run_dev` Signed-off-by: Phillip Rak <rak.phillip@gmail.com>

ericpromislow

UI Console Errors at startup :

./node_modules/@formatjs/icu-messageformat-parser/node_modules/tslib/tslib.es6.js
Module build failed: Error: ENOENT: no such file or directory, open '/Users/ericp/workspace/rancher/desktop/node_modules/@formatjs/icu-messageformat-parser/node_modules/tslib/tslib.es6.js'
./node_modules/@formatjs/icu-skeleton-parser/node_modules/tslib/tslib.es6.js
Module build failed: Error: ENOENT: no such file or directory, open '/Users/ericp/workspace/rancher/desktop/node_modules/@formatjs/icu-skeleton-parser/node_modules/tslib/tslib.es6.js'
./node_modules/intl-messageformat/node_modules/tslib/tslib.es6.js
Module build failed: Error: ENOENT: no such file or directory, open '/Users/ericp/workspace/rancher/desktop/node_modules/intl-messageformat/node_modules/tslib/tslib.es6.js'

ericpromislow · 2023-07-18T23:42:25Z

README.md

-npm run build
-npm run package --publish=never
+yarn build
+yarn package --publish=never


Further down, at line 222, there's a reference to MacOS. It should be macOS

And line 308, of how its should be of how it's

I can follow up to fix that. I think it's unrelated to this PR.

I can't reproduce the problems I reported

ericpromislow

Everything looks good now. I can move from npm dev to yarn dev without needing to do a factory-reset on all three platforms.

rak-phillip force-pushed the feature/3723-yarn-migration branch 3 times, most recently from ca88d33 to 3c5c59d Compare July 6, 2023 21:12

rak-phillip requested review from mook-as, ericpromislow and adamkpickering July 6, 2023 22:34

rak-phillip changed the title ~~Feature/3723 yarn migration~~ Migrate Project from npm to Yarn Jul 6, 2023

rak-phillip marked this pull request as ready for review July 6, 2023 22:35

ericpromislow requested changes Jul 7, 2023

View reviewed changes

adamkpickering requested changes Jul 7, 2023

View reviewed changes

rak-phillip removed the request for review from mook-as July 10, 2023 18:12

rak-phillip requested review from ericpromislow and adamkpickering July 10, 2023 20:13

jandubois reviewed Jul 11, 2023

View reviewed changes

bats/tests/helpers/defaults.bash Outdated Show resolved Hide resolved

rak-phillip force-pushed the feature/3723-yarn-migration branch from f42b780 to 1e38e55 Compare July 11, 2023 19:54

rak-phillip requested a review from jandubois July 11, 2023 19:55

ericpromislow requested changes Jul 13, 2023

View reviewed changes

ericpromislow previously approved these changes Jul 14, 2023

View reviewed changes

rak-phillip dismissed ericpromislow’s stale review via 8ed1104 July 17, 2023 20:31

rak-phillip force-pushed the feature/3723-yarn-migration branch 2 times, most recently from 8ed1104 to 07b6272 Compare July 17, 2023 20:33

rak-phillip requested a review from ericpromislow July 17, 2023 20:33

adamkpickering previously approved these changes Jul 17, 2023

View reviewed changes

rak-phillip added 13 commits July 18, 2023 16:16

Migrate from package-lock to yarn.lock

3b010af

Update lockfile Signed-off-by: Phillip Rak <rak.phillip@gmail.com>

Update jest.config

9558497

Signed-off-by: Phillip Rak <rak.phillip@gmail.com>

Replace npm run with yarn

c02e53d

Signed-off-by: Phillip Rak <rak.phillip@gmail.com>

Set yarn resolutions for typescript & @types/ref-napi

0e02270

Co-authored-by: Mark Yen <3977982+mook-as@users.noreply.github.com> Signed-off-by: Phillip Rak <rak.phillip@gmail.com>

Replace remaining instances of npm run with yarn

6ce80db

Signed-off-by: Phillip Rak <rak.phillip@gmail.com>

Clean up docs for transition to yarn

bfe6d84

Signed-off-by: Phillip Rak <rak.phillip@gmail.com>

Fix lint warnings

c9fb93a

Signed-off-by: Phillip Rak <rak.phillip@gmail.com>

Update github workflows

9b02444

Replace `npm ci` with `yarn install --frozen-lockfile` Install yarn globally for cirrus Package with python Remove yarn upgrade step change yarn registry Increase timeout duration Update yarn commands Signed-off-by: Phillip Rak <rak.phillip@gmail.com>

Update installation instructions

419be68

Signed-off-by: Phillip Rak <rak.phillip@gmail.com>

BATS: Replace using_npm_run_dev with using_dev_mode

9269200

- BATS: Update `using_dev` to `using_dev_mode` - Replace remaining instances of `using_npm_run_dev` Signed-off-by: Phillip Rak <rak.phillip@gmail.com>

rak-phillip dismissed adamkpickering’s stale review via 9269200 July 18, 2023 23:20

rak-phillip force-pushed the feature/3723-yarn-migration branch from 07b6272 to 9269200 Compare July 18, 2023 23:20

ericpromislow previously requested changes Jul 18, 2023

View reviewed changes

ericpromislow reviewed Jul 18, 2023

View reviewed changes

rak-phillip mentioned this pull request Jul 19, 2023

Typos in README.md #5195

Closed

ericpromislow approved these changes Jul 19, 2023

View reviewed changes

ml8mr merged commit 6c76b71 into rancher-sandbox:main Jul 19, 2023

rak-phillip deleted the feature/3723-yarn-migration branch July 19, 2023 22:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate Project from npm to Yarn #5106

Migrate Project from npm to Yarn #5106

rak-phillip commented Jul 5, 2023 •

edited

Loading

rak-phillip commented Jul 6, 2023

ericpromislow commented Jul 7, 2023

ericpromislow left a comment

adamkpickering left a comment

rak-phillip commented Jul 7, 2023 •

edited

Loading

rak-phillip commented Jul 10, 2023

rak-phillip commented Jul 10, 2023

rak-phillip commented Jul 11, 2023

jandubois commented Jul 11, 2023

ericpromislow left a comment

ericpromislow left a comment

rak-phillip commented Jul 14, 2023

rak-phillip commented Jul 17, 2023

adamkpickering left a comment

ericpromislow left a comment

ericpromislow Jul 18, 2023

ericpromislow Jul 18, 2023

rak-phillip Jul 18, 2023

ericpromislow left a comment

Migrate Project from npm to Yarn #5106

Migrate Project from npm to Yarn #5106

Conversation

rak-phillip commented Jul 5, 2023 • edited Loading

🗒️ Notes

Changes Made

Footnotes

rak-phillip commented Jul 6, 2023

ericpromislow commented Jul 7, 2023

ericpromislow left a comment

Choose a reason for hiding this comment

adamkpickering left a comment

Choose a reason for hiding this comment

rak-phillip commented Jul 7, 2023 • edited Loading

rak-phillip commented Jul 10, 2023

Footnotes

rak-phillip commented Jul 10, 2023

rak-phillip commented Jul 11, 2023

jandubois commented Jul 11, 2023

ericpromislow left a comment

Choose a reason for hiding this comment

ericpromislow left a comment

Choose a reason for hiding this comment

rak-phillip commented Jul 14, 2023

rak-phillip commented Jul 17, 2023

adamkpickering left a comment

Choose a reason for hiding this comment

ericpromislow left a comment

Choose a reason for hiding this comment

ericpromislow Jul 18, 2023

Choose a reason for hiding this comment

ericpromislow Jul 18, 2023

Choose a reason for hiding this comment

rak-phillip Jul 18, 2023

Choose a reason for hiding this comment

ericpromislow left a comment

Choose a reason for hiding this comment

rak-phillip commented Jul 5, 2023 •

edited

Loading

rak-phillip commented Jul 7, 2023 •

edited

Loading