Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Invalidate cached dependencies on dependency updates #759

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

cameel
Copy link
Member

@cameel cameel commented Jan 25, 2025

Depends on #756.
Related to #757 and #758.

Our caching of dependencies is based only on the content of package.json. This means that CI is running with versions that were most recent when it was last updated. This PR adds the output of npm outdated to the cache key to invalidate the cache whenever new versions appear.

Note that this is still not foolproof:

  • Cache will get invalidated even if we would not install the new version. E.g. if we depend on x@^1.0.0, the release of [email protected] will still trigger it, even though we'd only pull the last version of the 1.x series.
  • We might miss an update if the newly released version is not the latest one. E.g. if we depend on x@^1.0.0 and [email protected] already exists, [email protected] will not trigger it.

Despite these flaws, this is probably good enough and better than the current situation. We update package.json on every release anyway so we already do notice such breakage eventually. This PR would just make it much more likely that it won't remain undiscovered until the very day of the next release.

This becomes more important if we decide to merge #758, because that change would make solc-js automatically switch to new dependencies even if their version indicates they're breaking. We'd need to more actively react by adding an upper limit when that happens.

@cameel cameel added has dependencies The PR depends on other PRs that must be merged first testing 🔨 labels Jan 25, 2025
@cameel cameel requested a review from r0qs January 25, 2025 03:19
@cameel cameel self-assigned this Jan 25, 2025
@cameel cameel force-pushed the invalidate-cache-on-dependency-updates branch from 3dc9ac8 to 18e897e Compare January 25, 2025 03:21
@coveralls
Copy link

coveralls commented Jan 25, 2025

Coverage Status

coverage: 84.537%. remained the same
when pulling 9a210ab on invalidate-cache-on-dependency-updates
into 95d3a5f on master.

@cameel cameel force-pushed the invalidate-cache-on-dependency-updates branch from 18e897e to 0a8224b Compare January 25, 2025 03:25
@cameel cameel changed the base branch from drop-nodejs-10 to master January 25, 2025 03:32
@cameel cameel force-pushed the invalidate-cache-on-dependency-updates branch from 0a8224b to 53019f1 Compare January 25, 2025 03:36
@cameel cameel force-pushed the invalidate-cache-on-dependency-updates branch from 53019f1 to 9a210ab Compare January 25, 2025 03:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has dependencies The PR depends on other PRs that must be merged first testing 🔨
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants