Skip to content

Comments

Cache eslint runs#5094

Open
strawburster wants to merge 1 commit intoDSpace:mainfrom
strawburster:eslint-cache
Open

Cache eslint runs#5094
strawburster wants to merge 1 commit intoDSpace:mainfrom
strawburster:eslint-cache

Conversation

@strawburster
Copy link

@strawburster strawburster commented Feb 7, 2026

Description

Cache eslint runs so that they will run faster in the future using a cache file in the ./.angular/cache folder.

This PR was PR-ovided by The Library Code

Instructions for Reviewers

Build the angular development image and run the linter on it:

tag=dspace/dspace-angular:eslint-cache
podman build . --tag $tag
time podman run --entrypoint '' $tag npm run lint

On my machine this is 0m3.840s

vs latest docker angular development build:

tag=dspace/dspace-angular:latest
podman pull $tag
time podman run --entrypoint '' $tag npm run lint

On my machine it is 1m25.251s

The image size is increased by 56.6 megabytes as a result of the cache being included, and the initial build time for the angular image will be increased by approximately 90 seconds.

List of changes in this PR:

  • Dockerfile: run linter as part of build process for development image
  • package.json: add options to lint:nobuild script to cache the eslint run

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.

@alanorth
Copy link
Contributor

alanorth commented Feb 9, 2026

Thanks @strawburster. Results for me on Linux with Node.js v24 on main:

Before:

  • npm run lint 92.26s user 3.11s system 154% cpu 1:01.88 total

After (first run to build cache, second run to test):

  • npm run lint 92.25s user 3.19s system 154% cpu 1:01.79 total
  • npm run lint 3.15s user 0.40s system 181% cpu 1.958 total

I rarely run lint in my day to day DSpace maintenance activities. The huge benefit here seems to be speeding up our CI runs. Are there any downsides?

P.S. props for using podman instead of Docker. I do too!

@tdonohue tdonohue added code task 1 APPROVAL pull request only requires a single approval to merge labels Feb 10, 2026
@tdonohue tdonohue moved this to 🙋 Needs Reviewers Assigned in DSpace 10.0 Release Feb 10, 2026
@strawburster
Copy link
Author

Hey, thanks for testing! Actually, I wasn't concerning myself with the CI pipeline, but just for local development. Do we use the main Dockerfile for CI? It says it's for development purposes. If we do, then, I don't think this change will actually improve speed because creating the lint cache takes just as much time, if not more, than linting already, so the time gained in the actual test portion is more than lost in the build portion.

I usually run the dev server and linter inside the container, and bind-mount just the source code, that way I can avoid installing node_modules dependencies on my host system. The point of adding the lint-cache command to the dockerfile was so that I could easily delete and recreate containers, with the cache file stored in the image, and the first lint task I run will be very fast.

One of the issues I ran into is that the metadata between the image files and the bind-mount files is usually not correct, so the cache-strategy must be set to 'content' to ensure that it is used during a bind-mount. And, instead of dirtying the worktree more with the cache file, I moved it to the .angular folder. This makes it easy to exclude the angular build cache and the eslint cache at once from a bind mount. In the default case, where the eslintcache is stored as a file in the root of the project, it's impossible to exclude a single file from a bind-mount while you can ignore whole folders.

@alanorth
Copy link
Contributor

Hi @strawburster. I don't use containers for development but your approach sounds reasonable.

Add eslint cache to docker image, even if there are linting
errors.  The cache is stored in the .angular/cache folder.
@strawburster
Copy link
Author

I apologize, I tried to add the --force flag to the lint script in the dockerfile, but it wasn't passed correctly, I thought I had tested it but apparently not.

I updated the original comment because the statistics changed after I changed the options.

@strawburster
Copy link
Author

Hi @strawburster. I don't use containers for development but your approach sounds reasonable.

The container scripts were given to help review this PR, but the changes to the lint scripts in the package.json will affect all developers that lint locally, whether or not they use docker.

@strawburster
Copy link
Author

Is the AuthService test flaky? I really doubt this change caused any meaningful difference in build output.

@alanorth
Copy link
Contributor

Is the AuthService test flaky? I really doubt this change caused any meaningful difference in build output.

@strawburster lots of GitHub CI runs have intermittent failures. Let's restart the job to see if it finishes this time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1 APPROVAL pull request only requires a single approval to merge code task

Projects

Status: 🙋 Needs Reviewers Assigned

Development

Successfully merging this pull request may close these issues.

3 participants