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

Future/pnpm #811

Merged
merged 14 commits into from
Mar 6, 2024
Merged

Future/pnpm #811

merged 14 commits into from
Mar 6, 2024

Conversation

mattcorner
Copy link
Contributor

@mattcorner mattcorner commented Feb 3, 2024

Changes

Changes from using npm as a package manager to pnpm.

Benefits:

  • Code does not have access to dependencies of dependencies. The package.json must explicitly list all dependencies used in code.
  • More disk efficient. Symlinks node_modules to a global store.
  • More performant. Keeps a global store so if a dependency already exists, it does not need to redownload.
  • Github Action caching - Keep a global store for github actions.

More detail on Github Action Caching:
The cache key is ${{ runner.os }}-pnpm-store-${{ hashFiles('**/pnpm-lock.yaml') }}.

GitHub will remove any cache entries that have not been accessed in over 7 days. There is no limit on the number of caches you can store, but the total size of all caches in a repository is limited to 10 GB. Once a repository has reached its maximum cache storage, the cache eviction policy will create space by deleting the oldest caches in the repository.

Hence as long as no pnpm-lock.yaml file have not changed, then we will hit the cache.

Dependencies

UI/UX

Testing notes

All developers will need to install pnpm. There are many methods to doing so: https://pnpm.io/installation

Author checklist before assigning a reviewer

  • Reviewed my own code-diff.
  • PR assigned to me or an appropriate delegate.
  • Relevant labels added to the PR.
  • Appropriate tests have been added.
  • Lint and test workflows pass.

@mattcorner mattcorner force-pushed the future/pnpm branch 2 times, most recently from 41696de to b6b7eba Compare February 3, 2024 21:31
@mattcorner mattcorner self-assigned this Feb 20, 2024
@mattcorner mattcorner added enhancement New feature or request dependencies Pull requests that update a dependency file labels Feb 20, 2024
@dmbarry86
Copy link
Contributor

@mattcorner would you like me to have a bash with this?

@mattcorner
Copy link
Contributor Author

Couple more bits to wrap my head around, but i'll send it you way when ready.

@mattcorner mattcorner marked this pull request as draft February 22, 2024 10:17
@mattcorner mattcorner requested a review from dmbarry86 February 27, 2024 08:52
@mattcorner mattcorner marked this pull request as ready for review February 27, 2024 08:52
@dmbarry86
Copy link
Contributor

I assume these are dependencies of dependencies and not something we can control?

image

Copy link
Contributor

@dmbarry86 dmbarry86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Let's just have a quick chat tomorrow before merging to align on couple of things.

@mattcorner
Copy link
Contributor Author

I assume these are dependencies of dependencies and not something we can control?

image

Dependencies of plotly.js iirc.

@dmbarry86
Copy link
Contributor

I think we're good to go with this now @mattcorner? I assume your prerelease worked as expected?

@mattcorner mattcorner merged commit 360c57b into main Mar 6, 2024
3 checks passed
@mattcorner mattcorner deleted the future/pnpm branch March 6, 2024 09:48
@dmbarry86 dmbarry86 mentioned this pull request May 1, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants