rhino icon indicating copy to clipboard operation
rhino copied to clipboard

Replace `yarn` with `npm`

Open kamilzyla opened this issue 3 years ago • 1 comments

It's good to have as little dependencies as possible. Alternatively we could at least warn user that yarn is not available and what can be done about this.

This change should be easy to introduce in the future (in terms of backwards-compatibility). Potential drawback should be considered though: perhaps yarn works much faster than npm, or maybe there is some other good reason to prefer it.

kamilzyla avatar Mar 03 '22 12:03 kamilzyla

I tried doing this too,

yarn vs npm

Here are my changes: https://github.com/Appsilon/rhino/compare/main...feat/change-yarn-to-npm

The shift from yarn to npm seemed to be straightforward. The syntax was different as npm required that extra run command.

The main issue for me was setting the current working directory. For yarn it was --cwd, for npm, I learned it was --prefix. Correct me if I am wrong, as this is my first time playing around with yarn and npm.

Running the scripts also required an extra -- when setting arguments for the commands, e.g. lint-js when fix = TRUE had to be run as lint-js -- --fix.

npm install error

The problem here though is npm_install().

As I have noted, it is not working yet. By default, when I run npm install in terminal or via system2() it tries to find package.json in root. It throws an error because it should use package.json in .rhino/node.

I tried using --prefix and --nodedir but it still tries to find package.json in root. Trying npm install .rhino/node returns this error npm ERR! Cannot read properties of undefined (reading 'spec')

The only way I was able to rebuild node_modules and create package-lock.json is by running

cd .rhino/node && npm install

I did this in terminal and had to cd ../.. later to return to root, which is quite impractical. I also don't know how to do this via system2().

Dependency Warnings

I am also getting this errors when rebuilding modules. Maybe we need some updates on the dependencies?

npm WARN deprecated @stylelint/[email protected]: Use the original unforked package instead: postcss-markdown
npm WARN deprecated [email protected]: The querystring API is considered Legacy. new code should use the URLSearchParams API instead.

vibalre avatar Oct 27 '22 05:10 vibalre

Thanks @vibalre for experimenting with this! It's possible that --prefix won't do and we'll actually need to change the working directory whenever we run npm (perhaps using withr::with_dir().

kamilzyla avatar Oct 28 '22 14:10 kamilzyla