search

brianc / node-pg-types

Type parsing for node-postgres
267 stars 54 forks source link

ci: use npm ci #157

Closed alxndrsn closed 4 months ago

alxndrsn commented 4 months ago

Also add package-lock.json.

bendrucker commented 4 months ago

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.

sehrope commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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.