[8.x] yarn.lock: run yarn upgrade

[alanorth]

Description

Run yarn upgrade to force yarn to re-evaluate semver pins in package.json. This reveals some breaking changes in our dependencies, for example one new lint error due to a much newer typescript-eslint/eslint-plugin version (7.7.0 → 7.18.0).

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:

• Run yarn upgrade which makes many changes in yarn.lock
• Fix new lint error

Include guidance for how to test or review your PR.

Try to run yarn install, yarn lint, and build the site in dev and prod modes.

Checklist

This checklist provides a reminder of what we are going to look for when reviewing your PR. You do not need to complete this checklist prior 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!

• My PR is created against the main branch of code (unless it is a backport or is fixing an issue specific to an older branch).
• My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
• My PR passes ESLint validation using npm run lint
• My PR doesn't introduce circular dependencies (verified via npm run check-circ-deps)
• My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
• My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
• My PR aligns with Accessibility guidelines if it makes changes to the user interface.
• My PR uses i18n (internationalization) keys instead of hardcoded English text, to allow for translations.
• My PR includes details on how to test it. I've provided clear instructions to reviewers on how to successfully test this fix or feature.
• If my PR includes new libraries/dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.
• If my PR includes new features or configurations, I've provided basic technical documentation in the PR itself.
• If my PR fixes an issue ticket, I've linked them together.

[tdonohue]

@alanorth : It's worth noting that I've found that yarn upgrade is not always great at finding versions of dependencies that actually will work well together. Ideally, it should work, but every NPM dependency unfortunately is different...and some will add breaking changes in releases that are supposed to be minor.

It sounds like that's what you discovered here with eslint...even though (based on semver) version 7.18.0 should be compatible with version 7.7.0, it obviously is not.

[alanorth]

@tdonohue yarn's dependency solver is only evaluating version metadata from our dependencies using the version strings we specified. So the problem is "ours" in that we may have semver strings that aren't correct, for example because we forgot to upgrade one dependency when upgrading Angular, or because an upstream introduced breaking changes in a minor release (the case of @typescript-eslint/eslint-plugin). The NPM ecosystem is better at this than Java, for example, but still not perfect.

Since 8.0 was recently released I wonder if it's worth doing a yarn upgrade and dealing with the consequences early in 8.1-next. The problems I highlighted here will come to the fore eventually...

[tdonohue]

@alanorth : I'd welcome cleanup of our package.json. But, I was pointing out the @typescript-eslint/eslint-plugin version of ^7.2.0 seems like it would be correct if eslint-plugin was following semver, which says MINOR version when you add functionality in a backward compatible manner.

So, you are correct that we could improve the package.json. I'd welcome that, but I suspect it won't be easy. Currently, as you've seen, we have had a tendency to default to installing dependencies using Caret (^). It sounds like you're recommending we do a more thorough analysis of our dependencies to determine which should switch to using Tilde (~) instead of always using that Caret approach. (E.g. eslint-plugin may need to move to using Tilde, or we have to be ready to fix potential breakages whenever we upgrade.)

It's basically this sort of discussion: https://www.reddit.com/r/npm/comments/17boc2e/to_caret_or_not_to_caret/

Nonetheless, if we can get this cleaned up, I do agree it has the potential to make quick upgrades easier.

[alanorth]

Yeah it's a bit of a slog. Well the issues will come out eventually.

The big question is when do we as a project run yarn upgrade? The ecosystem—ie, our dependencies and their dependencies and onward—is moving, and keeping our lockfile static for months at a time is kicking the proverbial can down the road. I would argue that we need a better dependency bot (like Renovate) to be aggressively proposing upgrades in the lockfile and letting our tests in CI unearth issues.

[tdonohue]

@alanorth : I'd welcome digging into a better bot option. Currently, we are not running "yarn upgrade" frequently...and we tend to only upgrade dependencies when GitHub's dependabot notifies us of a security issue & sends a fix.

But, I'd welcome looking into whether we can have a bot automatically send us bug fix updates as well.

[tdonohue]

@alanorth : Regarding the lint error in this PR, we might need to upgrade eslint-plugin-unused-imports to 3.2.0 as noted here: vercel/style-guide#107 (comment)

[alanorth]

we might need to upgrade eslint-plugin-unused-imports to 3.2.0

Yep, we ran into that on main already. Let me add it here too...

I'd welcome digging into a better bot option

Renovate is the clear candidate, but it's pretty complex. I will try to have a go at a working configuration in a private fork.

[tdonohue]

👍 Thanks @alanorth ! I gave this a local test and verified that nothing seems to have broken. I did a full: yarn clean; yarn install; yarn build:prod; yarn serve:ssr and everything still is working properly in the User Interface. So, this yarn upgrade was successful (after those minor fixes were made).

Merging for 8.1