search

tristanls / dynamodb-lock-client

A general purpose distributed locking library built for AWS DynamoDB.
MIT License
50 stars 20 forks source link

Dependency Problem with "@hapi/joi" #19

Open simlu opened 5 years ago

simlu commented 5 years ago

And that's why it would be great to have at least some tests :)

We're working on a pr to fix this now...

simlu commented 5 years ago

There is a pr, however it keeps the joi dependency pinned...

Any objections to making this a peer dependency?

tristanls commented 5 years ago

Thank you and @deathgrindfreak for the quick notification and fixes.

I agree that tests would be nice. Unfortunately, tests would not have prevented me from publishing a broken version. For some reason a random joi@13.0.0 and all other dependencies are hanging out in my global environment. Before publishing, I did do the "sanity check" of $ node index.js which should break if dependencies were invalid, but it passed even after I removed node_modules folder to attempt to duplicate the problem.

Ironically, a reason to pin the version is to prevent failures such as the one introduced by v0.5.1 of this module.

Regarding peer dependency, the dependency on @hapi/joi is self contained and isolated within the module. As such, the typical use case of supporting an interface where the module user must pass in peer-version-specific parameters to the module does not occur. Without that, it seems like a toilsome user experience for module users to read a warning and install @hapi/joi themselves.

simlu commented 5 years ago

I agree that tests would be nice. Unfortunately, tests would not have prevented me from publishing a broken version. For some reason a random joi@13.0.0 and all other dependencies are hanging out in my global environment. Before publishing, I did do the "sanity check" of $ node index.js which should break if dependencies were invalid, but it passed even after I removed node_modules folder to attempt to duplicate the problem.

Tests and setting up ci/cd. I use that for all my npm packages and that actually just caught something today where an environment variable was only set locally.

You could also just run your tests locally in a docker container. Super simple to spawn a container with a shell in the project directory with e.g.

docker run -u`id -u`:`id -g` -v $(pwd):/user/project -v ~/.aws:/user/.aws -v ~/.npmrc:/user/.npmrc -w /user/project -it --entrypoint /bin/bash circleci/node:12

Note: remove/add file links as appropriate from/to that command

Ironically, a reason to pin the version is to prevent failures such as the one introduced by v0.5.1 of this module.

Pinning is not really necessary if you're using a lock file in your repo, which you probably should, since that is the only way to include sub dependency versions. But if you prefer to pin - which I do as well for smaller, non-peer dependencies - auto dependency updates are the way to go (e.g. use dependabot).

But separate from that, I would strongly encourage you to add a lock file to this project to really prevent the kind of failures you were talking about!

Regarding peer dependency, the dependency on @hapi/joi is self contained and isolated within the module. As such, the typical use case of supporting an interface where the module user must pass in peer-version-specific parameters to the module does not occur. Without that, it seems like a toilsome user experience for module users to read a warning and install @hapi/joi themselves.

I consider @hapi/joi one of the "default" dependencies (check out joi-strict btw) and hence prefer to not have multiple installs/version floating around in my node_modules folder. But I guess it is not that big at 100kb. Unfortunately we really have to watch for package size these days... Hence my desire to make it a peer dependency. Similar to the aws-sdk dependency. That should always be a peer dependency and never a dev dependency.

Hope that clarified some things. Please let me know if you have any questions! Always happy to discuss my reasoning and best practices :)

tristanls commented 5 years ago

package-lock.json

I honestly don't understand the utility you get out of package-lock.json. Only immediate dependencies can be pinned with or without package-lock.json. package-lock.json gets overridden every time that package.json is updated (if versions are pinned), or every time npm upgrade is executed (if versions are ranges like ^15.1.0). This override and update to latest happens even if "locked" dependencies satisfy new dependencies, so what is the point?

In the example below note that the "locked" @hapi/hoek version (the one that @hapi/topo depends on) will be updated after updating from @hapi/joi@15.1.0 to @hapi/joi@15.1.1.

package-lock.json only "locks" things if package.json doesn't change. 

$ cat package.json 
{
  "name": "pLock",
  "version": "0.0.0",
  "dependencies": {
    "@hapi/joi": "15.1.0"
  }
}
$ cat package-lock.json 
{
  "name": "pLock",
  "version": "0.0.0",
  "lockfileVersion": 1,
  "requires": true,
  "dependencies": {
    "@hapi/address": {
      "version": "2.0.0",
      "resolved": "https://registry.npmjs.org/@hapi/address/-/address-2.0.0.tgz",
      "integrity": "sha512-mV6T0IYqb0xL1UALPFplXYQmR0twnXG0M6jUswpquqT2sD12BOiCiLy3EvMp/Fy7s3DZElC4/aPjEjo2jeZpvw=="
    },
    "@hapi/hoek": {
      "version": "6.2.4",
      "resolved": "https://registry.npmjs.org/@hapi/hoek/-/hoek-6.2.4.tgz",
      "integrity": "sha512-HOJ20Kc93DkDVvjwHyHawPwPkX44sIrbXazAUDiUXaY2R9JwQGo2PhFfnQtdrsIe4igjG2fPgMra7NYw7qhy0A=="
    },
    "@hapi/joi": {
      "version": "15.1.0",
      "resolved": "https://registry.npmjs.org/@hapi/joi/-/joi-15.1.0.tgz",
      "integrity": "sha512-n6kaRQO8S+kepUTbXL9O/UOL788Odqs38/VOfoCrATDtTvyfiO3fgjlSRaNkHabpTLgM7qru9ifqXlXbXk8SeQ==",
      "requires": {
        "@hapi/address": "2.x.x",
        "@hapi/hoek": "6.x.x",
        "@hapi/marker": "1.x.x",
        "@hapi/topo": "3.x.x"
      }
    },
    "@hapi/marker": {
      "version": "1.0.0",
      "resolved": "https://registry.npmjs.org/@hapi/marker/-/marker-1.0.0.tgz",
      "integrity": "sha512-JOfdekTXnJexfE8PyhZFyHvHjt81rBFSAbTIRAhF2vv/2Y1JzoKsGqxH/GpZJoF7aEfYok8JVcAHmSz1gkBieA=="
    },
    "@hapi/topo": {
      "version": "3.1.3",
      "resolved": "https://registry.npmjs.org/@hapi/topo/-/topo-3.1.3.tgz",
      "integrity": "sha512-JmS9/vQK6dcUYn7wc2YZTqzIKubAQcJKu2KCKAru6es482U5RT5fP1EXCPtlXpiK7PR0On/kpQKI4fRKkzpZBQ==",
      "requires": {
        "@hapi/hoek": "8.x.x"
      },
      "dependencies": {
        "@hapi/hoek": {
          "version": "8.2.0",
          "resolved": "https://registry.npmjs.org/@hapi/hoek/-/hoek-8.2.0.tgz",
          "integrity": "sha512-pR2ZgiP562aiaQvQ98WgfqfTrm+xG+7hwHRPEiYZ+7U1OHAAb4OVZJIalCP03bMqYSioQzflzVTVrybSwDBn1Q=="
        }
      }
    }
  }
}

Updating to 15.1.1

{
  "name": "pLock",
  "version": "0.0.0",
  "dependencies": {
    "@hapi/joi": "15.1.1"
  }
}
$ npm i
npm WARN pLock@0.0.0 No description
npm WARN pLock@0.0.0 No repository field.
npm WARN pLock@0.0.0 No license field.

added 1 package, removed 2 packages, updated 2 packages and audited 6 packages in 2.73s
found 0 vulnerabilities
{
  "name": "pLock",
  "version": "0.0.0",
  "lockfileVersion": 1,
  "requires": true,
  "dependencies": {
    "@hapi/address": {
      "version": "2.0.0",
      "resolved": "https://registry.npmjs.org/@hapi/address/-/address-2.0.0.tgz",
      "integrity": "sha512-mV6T0IYqb0xL1UALPFplXYQmR0twnXG0M6jUswpquqT2sD12BOiCiLy3EvMp/Fy7s3DZElC4/aPjEjo2jeZpvw=="
    },
    "@hapi/bourne": {
      "version": "1.3.2",
      "resolved": "https://registry.npmjs.org/@hapi/bourne/-/bourne-1.3.2.tgz",
      "integrity": "sha512-1dVNHT76Uu5N3eJNTYcvxee+jzX4Z9lfciqRRHCU27ihbUcYi+iSc2iml5Ke1LXe1SyJCLA0+14Jh4tXJgOppA=="
    },
    "@hapi/hoek": {
      "version": "8.2.1",
      "resolved": "https://registry.npmjs.org/@hapi/hoek/-/hoek-8.2.1.tgz",
      "integrity": "sha512-JPiBy+oSmsq3St7XlipfN5pNA6bDJ1kpa73PrK/zR29CVClDVqy04AanM/M/qx5bSF+I61DdCfAvRrujau+zRg=="
    },
    "@hapi/joi": {
      "version": "15.1.1",
      "resolved": "https://registry.npmjs.org/@hapi/joi/-/joi-15.1.1.tgz",
      "integrity": "sha512-entf8ZMOK8sc+8YfeOlM8pCfg3b5+WZIKBfUaaJT8UsjAAPjartzxIYm3TIbjvA4u+u++KbcXD38k682nVHDAQ==",
      "requires": {
        "@hapi/address": "2.x.x",
        "@hapi/bourne": "1.x.x",
        "@hapi/hoek": "8.x.x",
        "@hapi/topo": "3.x.x"
      }
    },
    "@hapi/topo": {
      "version": "3.1.3",
      "resolved": "https://registry.npmjs.org/@hapi/topo/-/topo-3.1.3.tgz",
      "integrity": "sha512-JmS9/vQK6dcUYn7wc2YZTqzIKubAQcJKu2KCKAru6es482U5RT5fP1EXCPtlXpiK7PR0On/kpQKI4fRKkzpZBQ==",
      "requires": {
        "@hapi/hoek": "8.x.x"
      }
    }
  }
}

The only thing I've experienced package-lock.json is useful for is addressing vulnerable dependencies of dependencies. And in that case, the package-lock.json of the code using this module suffices. This is the reason why I punt on including package-lock.json in "library" code like this, and let the user manage it in their package-lock.json.

size

Yeah, I can see how shipping all of @hapi/joi versions around would not be great. I think that's a good argument for depending on a major version 15.x.x or maybe a minor version 15.1.x instead of pinning. But that's really not the peerDependencies use case.

peerDependencies

On the other hand, while I don't think @hapi/joi is a peerDependency, the module does accept AWS.DynamoDB.DocumentClient as part of its configuration, which seems like the exact use case to specify which version of that client the module expects. Thus, I should probably add aws-sdk into peerDependencies.