Use npm instead of Yarn #3173

alanorth · 2024-07-08T12:38:19Z

References

Fixes Upgrade to Yarn v2 or switch back to npm #1074

Description

Simplify the project by using npm (which ships with Node.js) instead of Yarn (an extra global dependency and prerequisite).

The original reasons for using Yarn (dependency install speed and robust lockfiles) are no longer as compelling. In the past few years npm has improved in both of these areas. Furthermore, we are using Yarn 1.x "classic", which has been deprecated for several years and migration to Yarn 2, which is not backwards compatible, will require us to do some work anyway.

Instructions for Reviewers

Please add a more detailed description of the changes made by your PR. At a minimum, providing a bulleted list of changes in your PR is helpful to reviewers.

List of changes in this PR:

Remove yarn.lock
Update package.json to use npm run for all scripts
Add package-lock.json
Update several dependencies to fix lint errors
Update Dockerfiles to use npm
Update README.md with new instructions for npm

Include guidance for how to test or review your PR.

Try installing dependencies and building the application in dev and production mode:

$ rm -rf node_modules && npm install
$ npm run build
$ npm start

Checklist

This checklist provides a reminder of what we are going to look for when reviewing your PR. You need not complete this checklist prior to creating your PR (draft PRs are always welcome).
However, reviewers may request that you complete any actions in this list if you have not done so. If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!

tdonohue · 2024-07-11T20:03:47Z

Thanks for this work @alanorth ! I can see we'll need to do more work here to update other areas where yarn is used (namely the GitHub Actions, Docker scripts & README). But, this looks like a good start!

I've linked this over to the 9.0 brainstorms we've started gathering for Steering to discuss. I see this as a task which can help with the "Easier Installation" idea (by removing a prerequisite in yarn).

@artlowel and @atarix83 : I added you both as reviewers here as I'd appreciate your feedback on this idea. I'd be willing to help @alanorth to get the GitHub Actions / Docker scripts updated, assuming we all agree with the goal of removing usage of yarn in favor of npm. So, please let me know if you have immediate objections/concerns.

pnbecker · 2024-07-15T16:22:49Z

I like this idea a lot.

artlowel

Thanks @alanorth!

I agree, getting rid of yarn 1 is a good idea.

This PR does seem to break our lint step (On github you see the yarn version, but I get the same issue running lint with npm). Since it won't compile, perhaps eslint resolves to a different version now?

You can get rid of the resolutions section in package.json, it's something unique to yarn, to force to use a certain version of a dependency when there are conflicts. Since the project builds fine, it looks like these particular resolutions are no longer necessary anyway.

I do think we should add the npm equivalent, which is called overrides, and solve any conflict we have with peerDependencies in there, rather than instructing people to run npm install with --legacy-peer-deps

We'll also need to update the github workflows to use npm instead of yarn, as well as the Dockerfile

alanorth · 2024-08-29T18:12:17Z

Thanks @artlowel!

This PR does seem to break our lint step (On github you see the yarn version, but I get the same issue running lint with npm). Since it won't compile, perhaps eslint resolves to a different version now?

Oh I'm not sure, that needs more investigation. Appreciate any advice.

You can get rid of the resolutions section in package.json, it's something unique to yarn, to force to use a certain version of a dependency when their are conflicts. Since the project builds fine, it looks like these particular resolutions are no longer necessary anyway.

Done, thanks.

I do think we should add the npm equivalent, which is called overrides, and solve any conflict we have with peerDependencies in there, rather than instructing people to run npm install with --legacy-peer-deps

Oh that is nice to know. I have worked through the conflicting dependencies and added overrides for each. Now we can install with npm install!

We'll also need to update the github workflows to use npm instead of yarn, as well as the Dockerfile

Ah yes, true. Should be easy. I will take a look soon.

github-actions · 2024-08-30T16:59:55Z

Hi @alanorth,
Conflicts have been detected against the base branch.
Please resolve these conflicts as soon as you can. Thanks!

github-actions · 2024-09-03T21:38:06Z

Hi @alanorth,
Conflicts have been detected against the base branch.
Please resolve these conflicts as soon as you can. Thanks!

kshepherd · 2024-09-04T11:55:17Z

@alanorth I fixed the lint errors with an update to the unused-imports plugin (this was causing the problem, not the spec test itself) - "eslint-plugin-unused-imports": "^4.1.3", does it, though note that the documentation says 4.x.x is for eslint 9, eslint-plugins 8-5 (we are behind this right?), so I think to be sure we should review all the eslint versions and see what we can update?

doc:
https://www.npmjs.com/package/eslint-plugin-unused-imports

github-actions · 2024-09-04T21:52:09Z

Hi @alanorth,
Conflicts have been detected against the base branch.
Please resolve these conflicts as soon as you can. Thanks!

Instead of using the Yarn link protocol, we can use npm's built-in support for local dependencies using "file:".

alanorth · 2024-09-05T06:05:12Z

Thanks @kshepherd! You're right, the eslint-plugin-unused-imports say this:

Version 4.1.x is for eslint 9 with @typescript-eslint/eslint-plugin v8 to v5

Version 3.x.x is for eslint 8 with @typescript-eslint/eslint-plugin 6 - 7

Version 2.x.x is for eslint 8 with @typescript-eslint/eslint-plugin 5

We are using 2.x.x currently, but according to this we should be using 3.x.x. It fixes the lint error, though I'm not sure why we don't hit that issue with yarn. 🤔

All scripts should use "npm run" instead of "yarn".

Now that we are using npm we need to commit this.

These are a yarn-specific concept that we don't need for npm.

Add overrides for conflicting peer dependencies. In yarn these were handled less noisily, but in npm we need to add explicit overrides for each. This allows us to use `npm install` without forcing or using `--legacy-peer-deps`.

artlowel

Thanks @alanorth!

This is looking good to me now, I have one more minor inline comment.

It would be good if someone with more experience with Docker could give those changes a more thorough test though

.github/workflows/build.yml

Use npm instead of yarn in GitHub Actions.

Fix error in lint: > lint/src/util/structure.ts:16:20 - error TS2314: Generic type 'RuleMetaData<MessageIds, Options>' requires 2 type argument(s). > > 16 export type Meta = RuleMetaData<string>;

Fix lint error. According to the docs we should be using v3.x.x: > * Version 3.x.x is for eslint 8 with @typescript-eslint/eslint-plugin 6 - 7 > * Version 2.x.x is for eslint 8 with @typescript-eslint/eslint-plugin 5 See: https://www.npmjs.com/package/eslint-plugin-unused-imports

There is a minor difference in syntax when starting the application with `npm run serve`. We use `--` to make sure npm doesn't try to parse the `--host 0.0.0.0` as an option, allowing it instead to be read by Angular CLI (ng). Also, there is no direct npm equivalent to yarn's --network-timeout option so I will remove it. If we need something similar later we might be able to use these config options: npm config set fetch-retry-mintimeout 300000 npm config set fetch-retry-maxtimeout 300000

This was the only Angular package not using a "^" to specify the semver range.

We need this now that we are using npm instead of yarn.

Use npm instead of yarn.

Use npm instead of yarn. Mostly a search and replace, minus some changes in terminology, for example "yarn add" vs "npm install".

kshepherd

+1 looks good, tests well (I use docker and all seems good in my environment), thanks @alanorth

alanorth · 2024-09-06T12:13:15Z

Thanks for testing @artlowel and @kshepherd. This is at +2 now. As far as I can see the only thing remaining is to update the DSpace wiki, though the section for DSpace 9 doesn't exist yet.

tdonohue

👍 Thanks @alanorth ! This was waiting on me to do a quick review. This looks GREAT! It's working well for me. I noticed on minor thing (and already fixed it), where the yarn.lock should now be listed in our .gitignore and .dockerignore in order to ensure it's no longer added back to our codebase.

Overall, this is ready to merge for 9.0. I'll leave the needs documentation flag in place until we create the new 9.0 wiki space, as we'll need to (eventually) update the docs there.

alanorth added improvement dependencies Pull requests that update a dependency file needs documentation PR is missing documentation. All new features and config changes require documentation. labels Jul 8, 2024

github-actions bot assigned alanorth Jul 8, 2024

alanorth requested a review from artlowel July 8, 2024 12:40

tdonohue requested review from atarix83 and tdonohue July 9, 2024 14:19

artlowel requested changes Aug 29, 2024

View reviewed changes

alanorth force-pushed the use-npm branch from 752cad1 to f030367 Compare August 29, 2024 16:25

github-actions bot added the merge conflict label Aug 30, 2024

alanorth force-pushed the use-npm branch from 0a6819e to 94741ac Compare September 1, 2024 18:30

github-actions bot removed the merge conflict label Sep 1, 2024

github-actions bot added the merge conflict label Sep 3, 2024

alanorth force-pushed the use-npm branch from 94741ac to 664fd40 Compare September 4, 2024 05:22

github-actions bot removed the merge conflict label Sep 4, 2024

alanorth force-pushed the use-npm branch from 45747fb to 6e86518 Compare September 4, 2024 06:05

github-actions bot added the merge conflict label Sep 4, 2024

alanorth added 2 commits September 5, 2024 08:40

Remove yarn.lock

7fc54ec

package.json: use "file:" for local dependencies

84c88a1

Instead of using the Yarn link protocol, we can use npm's built-in support for local dependencies using "file:".

alanorth force-pushed the use-npm branch from 3ff8fe8 to 457474a Compare September 5, 2024 06:05

github-actions bot removed the merge conflict label Sep 5, 2024

package.json: update scripts to use "npm run"

74ac315

All scripts should use "npm run" instead of "yarn".

alanorth added 3 commits September 5, 2024 10:30

.gitignore: remove package-lock.json

ce3285c

Now that we are using npm we need to commit this.

package.json: remove resolutions

ee33b8f

These are a yarn-specific concept that we don't need for npm.

package.json: add overrides for conflicts

c6f984b

Add overrides for conflicting peer dependencies. In yarn these were handled less noisily, but in npm we need to add explicit overrides for each. This allows us to use `npm install` without forcing or using `--legacy-peer-deps`.

alanorth force-pushed the use-npm branch 4 times, most recently from 7610a49 to d9be88d Compare September 5, 2024 10:14

alanorth requested a review from artlowel September 5, 2024 10:15

artlowel approved these changes Sep 5, 2024

View reviewed changes

.github/workflows/build.yml Outdated Show resolved Hide resolved

alanorth added 8 commits September 5, 2024 14:56

.github/workflows/build.yml: use npm

da1789a

Use npm instead of yarn in GitHub Actions.

lint/src/util: fix TS2314

e29a6a3

Fix error in lint: > lint/src/util/structure.ts:16:20 - error TS2314: Generic type 'RuleMetaData<MessageIds, Options>' requires 2 type argument(s). > > 16 export type Meta = RuleMetaData<string>;

package.json: fix @angular/localize version pin

fbf2aee

This was the only Angular package not using a "^" to specify the semver range.

.dockerignore: remove package-lock.json

3894c87

We need this now that we are using npm instead of yarn.

.github/pull_request_template.md: use npm

d11e417

Use npm instead of yarn.

README.md: use npm

576aece

Use npm instead of yarn. Mostly a search and replace, minus some changes in terminology, for example "yarn add" vs "npm install".

alanorth force-pushed the use-npm branch from d9be88d to 576aece Compare September 5, 2024 11:56

tdonohue added this to the 9.0 milestone Sep 5, 2024

kshepherd approved these changes Sep 5, 2024

View reviewed changes

tdonohue added 2 commits September 6, 2024 12:33

Add yarn.lock to .dockerignore

202e765

Add yarn.lock to .gitignore

8144d1b

tdonohue approved these changes Sep 6, 2024

View reviewed changes

tdonohue added the high priority label Sep 6, 2024

tdonohue merged commit ba05b22 into DSpace:main Sep 6, 2024
13 checks passed

tdonohue mentioned this pull request Sep 6, 2024

Remove material-ui dependency #3171

Merged

11 tasks

alanorth deleted the use-npm branch September 7, 2024 16:40

This was referenced Sep 12, 2024

(Discussion) Easier Installation Process for DSpace #3184

Open

[#2718] Migrate cookie consent from Klaro to Orejime #3199

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use npm instead of Yarn #3173

Use npm instead of Yarn #3173

alanorth commented Jul 8, 2024 •

edited

Loading

tdonohue commented Jul 11, 2024

pnbecker commented Jul 15, 2024

artlowel left a comment •

edited

Loading

alanorth commented Aug 29, 2024 •

edited

Loading

github-actions bot commented Aug 30, 2024

github-actions bot commented Sep 3, 2024

kshepherd commented Sep 4, 2024 •

edited

Loading

github-actions bot commented Sep 4, 2024

alanorth commented Sep 5, 2024

artlowel left a comment

kshepherd left a comment

alanorth commented Sep 6, 2024

tdonohue left a comment

Use npm instead of Yarn #3173

Use npm instead of Yarn #3173

Conversation

alanorth commented Jul 8, 2024 • edited Loading

References

Description

Instructions for Reviewers

Checklist

tdonohue commented Jul 11, 2024

pnbecker commented Jul 15, 2024

artlowel left a comment • edited Loading

Choose a reason for hiding this comment

alanorth commented Aug 29, 2024 • edited Loading

github-actions bot commented Aug 30, 2024

github-actions bot commented Sep 3, 2024

kshepherd commented Sep 4, 2024 • edited Loading

github-actions bot commented Sep 4, 2024

alanorth commented Sep 5, 2024

artlowel left a comment

Choose a reason for hiding this comment

kshepherd left a comment

Choose a reason for hiding this comment

alanorth commented Sep 6, 2024

tdonohue left a comment

Choose a reason for hiding this comment

alanorth commented Jul 8, 2024 •

edited

Loading

artlowel left a comment •

edited

Loading

alanorth commented Aug 29, 2024 •

edited

Loading

kshepherd commented Sep 4, 2024 •

edited

Loading