search

digidem / comapeo-core

A local-first library for collaborating on mapping projects
MIT License
8 stars 1 forks source link

Consider publishing with `npm-shrinkwrap.json` #841

Open gmaclennan opened 2 months ago

gmaclennan commented 2 months ago

Description

npm-shrinkwrap.json has the same functionality as package-lock.json, but it is published with the package to npm. The npm docs say:

It's strongly discouraged for library authors to publish this file, since that would prevent end users from having control over transitive dependency updates.

However, we have seen multiple times in the past that changes to transitive dependencies break things. Our tests on @comapeo/core are very thorough, and should catch bugs caused by transitive dependency changes, however without a npm-shrinkwrap.json the apps that depend on @comapeo/core could easily be using different transitive dependencies to the ones that are tested here, and the tests in the apps are less thorough than here, which could result in runtime bugs.

Publishing npm-shrinkwrap.json will ensure that any app using @comapeo/core will be using exactly the same version that is tested here.

Tasks

EvanHahn commented 2 months ago

I'm pretty hesitant to do something that's "strongly discouraged". Could we get some of the same benefits if we pinned exact dependency versions in package.json? That wouldn't cover sub-dependencies but might be enough for us.

gmaclennan commented 2 months ago

The reasons for it being "strongly discouraged" are the exact reasons why we want it: it ensures that it is used in the app with exactly the same deps as we test on. The npm doc writers don't like the idea that an upstream dependent of the dep can't update a transitive dependency to fix a bug. We don't really want to be able to do that without running all the comapeo-core tests on that dependency update.

Pinning exact dependency versions goes a way to solving this, but not entirely. The issue in the mobile app was actually with the hypercore version depended on by corestore. It is perfectly possible that the mobile app could be using the pinned version of corestore, but the hypercore dep of corestore could be a different version (this could happen by an unsuspecting dev updating transitive deps to the latest matching semver).

EvanHahn commented 2 months ago

Makes sense to me!

Presumably we want to do this at release time. We wouldn't put npm-shrinkwrap.json into source control, and only include it in the final tarball. Is that what you're imagining?