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

ci: use npm ci #157

Closed
wants to merge 2 commits into from
Closed

ci: use npm ci #157

wants to merge 2 commits into from

Conversation

alxndrsn
Copy link
Contributor

@alxndrsn alxndrsn commented Apr 2, 2024

Also add package-lock.json.

alxndrsn added 2 commits April 2, 2024 13:15
Also add package-lock.json
@alxndrsn alxndrsn mentioned this pull request Apr 2, 2024
@bendrucker
Copy link
Collaborator

No thanks, lockfiles are for applications not libraries. This changes the behavior of the published package and we'd rather let the dependencies match a range as specified.

@bendrucker bendrucker closed this Apr 2, 2024
@sehrope
Copy link
Contributor

sehrope commented Apr 2, 2024

You can have both if you add the lockfile to the repo but exclude it from the published module. That way you get consistent build / CI but keep the range itself for the published package.

I don't think you'd have to add anything else either as the package.json already has an explicit list of files (so the package lock won't get included).

@bendrucker
Copy link
Collaborator

consistent build / CI but keep the range itself for the published package.

Why is this desirable though? The goal of CI is to detect problems before they reach production (i.e., the published package), not necessarily to be consistent.

In theory locking just the dev dependencies and not the regular ones would be ideal because we definitely do want consistent behavior from test tools. In practice, not worth the effort.

@sehrope
Copy link
Contributor

sehrope commented Apr 2, 2024

Having consistent packages for the tooling and the repo as a whole is useful for a number of reasons. Not the least of which is a known state when you git clone ... && npm ci && ... to test something out. Otherwise you never know what you're installing locally v.s. what CI is running or why it's breaking. It also prevents random CI failures when the latest version of some tooling package ends up breaking CI (e.g., the eslint stuff that happened to pg).

If you want to have a "canary" style build testing things out with the latest version of the matching deps, that can live in parallel with the regular CI process. It either delete the package lock or run a non-ci npm i to get the latest matching versions of packages. Nice thing about splitting that out is that you can even run it on a cron schedule so it happens daily or weekly. It doesn't have to be per-PR push and it lets you silence those notifications if they're too annoying (i.e. manually checking them).

It is a bit of a pain to manually update the lockfile, but you only really need to do it when you want to bump the baseline versions of those deps. And it becomes a manual checkpoint as you'd get CI to match up with whatever you tested out locally (e.g., bump eslint and verifying that it still works).

@bendrucker
Copy link
Collaborator

All this is a reasonably achievable workflow but the many paragraphs of explanation of the tooling involved hints at why it's not a reasonable default. It's a lot of work to address an easily solvable and rare problem for maintainers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants