Dependency management #302
Conversation
|
Thanks for this one. Just to let you know, we updated repositories in two github organizations to follow that practice related to CI. https://github.com/nodeshift And looks like we don't need to worry about update Node.js in CI (.travis.yml) until April 2021 ( for our use case ) 😄 |
|
|
||
| There is no single recipe for choosing the right dependency for your package, but there are some indicators to pay attention to and questions to ask yourself: | ||
|
|
||
| - Do you need a dependency at all? Each dependency has a download cost, and if it is build by someone else - it may have security and reliability implications. |
There was a problem hiding this comment.
I very much object to discouraging people from adding a dependency - one that is likely to be better tested and more widely used than your own alternative - based on download cost. Separately, first-party code has imo more risk in terms of security and reliability than most third party code.
There was a problem hiding this comment.
I understand the sentiment and I'm also a fan of small modules and doing one thing well, however this is not a one sided argument. But you're right - it's lazy of me to word this as a "download cost" problem alone. The security implications are a trade off and depend on the use case, e.g. there's no point to drag in the whole of chalk if you just want to make one line of text red and I would strongly disagree that looking for a module which only does the red colouring is worth it either. These costs are also passed on to the consumers of your package, so they add up eventually.
I would also strongly disagree on the first vs third party code - once again - it depends case-by-case, and there's authors I trust more than others, and I'd never write my own crypto, but at the same time - we have seen incidents, fairly recently, and the risk is there, so a piece of code that one has written themselves, for a specific use case, where they understand it 100% is easier to manage than to try to track every possible change in a dependency, which is likely not even related.
I'm sure we could debate this for ages.
I'll rephrase this to give a better overview of both perspectives.
There was a problem hiding this comment.
there's no point to drag in the whole of chalk if you just want to make one line of text red
I find plenty of point in doing it; that chalk does more than you need doesn't make it easy to correctly do the subset. For example, virtually every single complainer about "leftpad" who offered a one-liner had subtle bugs (i was moving String.prototype.padStart through TC39 at the time).
I'll rephrase this to give a better overview of both perspectives.
Thanks, I think a balanced discussion of tradeoffs is reasonable
There was a problem hiding this comment.
Agreed that the discussion should include more than "download cost". Things like the licence requirements, and level of ongoing maintenance/support is more important to me. I do agree though that some thought to the size of what you are pulling in does makes sense as well.
There was a problem hiding this comment.
Does acdd9c5 sound ok?
There was a problem hiding this comment.
Somewhat - it doesn't have implications because it was built by somebody else, it's that if you didn't build it, you have to audit and understand it in order to evaluate those possible implications.
In all cases, you have to understand the risks and understand what the code is doing; it's just that if you do it yourself, you're not sharing that risk with anyone else. In other words, I think that there is inherently more risk in reinventing wheels, and I'd like to see prose that speaks to that.
There was a problem hiding this comment.
Can you suggest improvements to the existing text?
There was a problem hiding this comment.
I think anything that implies that avoiding a dependency is something to strive for is inherently problematic.
How about replacing this item with some combination of:
Is this dependency the right tool for the job? Does it appear well-maintained, do you trust the authors/maintainers? Does it have thorough automated test coverage? If you had to fix a bug in it, could you? Is the problem it solves encapsulated well, so that it would be easy to replace this dependency with another if needed?
| - `package-lock.json` is only used in development, so you may be testing your package with a different set of dependency versions than your users will be using it, which may occasionally lead to unexpected results. | ||
| - no dependency updates will be received automatically with `npm install`. | ||
|
|
||
| You can disable the lock files in your global or local `.npmrc` file by adding a `package-lock=false` line. This does however mean that npm will also ignore the shrinkwraps included in your dependencies. |
There was a problem hiding this comment.
| You can disable the lock files in your global or local `.npmrc` file by adding a `package-lock=false` line. This does however mean that npm will also ignore the shrinkwraps included in your dependencies. | |
| You can disable npm's default lockfile in your global or local `.npmrc` file by adding a `package-lock=false` line. |
Removed the second sentence as afaik it is quite incorrect; if anything causes npm (or yarn) to ignore a published shrinkwrap file, npm is broken, and we should file an issue instead.
There was a problem hiding this comment.
This has worked like that since package-lock.json was introduced though?
This option is an alias for --shrinkwrap.
And setting it to false disables both - creation and usage of all the lock files.
I definitely think this behavior is not something I expect or find intuitive and it gets worse when you have package-lock and shrinkwrap options set to different values, so maybe yes - an issue on npm tracker is in order (if nobody has one created yet), but I'd say it is worth documenting this (with a link to the issue) until it is fixed?
There was a problem hiding this comment.
It disables creation but as far as I'm aware does not disable usage of nested npm-shrinkwrap files; a repro case would be interesting.
Either way, it's user-hostile to publish a shrinkwrap file (since it prevents deduping) so i doubt that it's a common enough case to warrant mentioning.
There was a problem hiding this comment.
Docs for shrinkwrap say:
If set to false, then ignore npm-shrinkwrap.json files when installing.
I'd have to double check, but I think it ignores the published shrinkwraps as well as the ones present in the folder next to package.json.
it's user-hostile to publish a shrinkwrap file (since it prevents deduping)
I don't think many people do that, and even hapi gave up the practice, but if the shrinkwraps are there - they will be ignored and it is not necessarily something people will expect?
There was a problem hiding this comment.
Perhaps, but i doubt the installer of a package has any expectation that there's a shrinkwrap file there - it seems worth mentioning to package authors who publish a shrinkwrap file, but I'd prefer to use that space to tell them not to publish one in the first place :-)
There was a problem hiding this comment.
OK, so I did a quick test with latest npm. When package-lock=false and a dependency has a shrinkwrap inside the tarball, the shrinkwrap is ignored. So I think the original statement is correct - package-lock=false disables all lockfiles - they will not be respected, nor will they be created, so the suggestion is not entirely correct and I think it's worth keeping the note that shrinkwraps will be ignored.
I've added a bullet point on deduplication: c126a7d
There was a problem hiding this comment.
| You can disable the lock files in your global or local `.npmrc` file by adding a `package-lock=false` line. This does however mean that npm will also ignore the shrinkwraps included in your dependencies. | |
| You can disable lockfiles in your global or local `.npmrc` file by adding a `package-lock=false` line. This does however mean that npm will also ignore the `npm-shrinkwrap.json` files included in your dependencies. |
Co-Authored-By: Liran Tal <liran.tal@gmail.com>
Co-Authored-By: Liran Tal <liran.tal@gmail.com>
Co-Authored-By: Jordan Harband <ljharb@gmail.com>
Co-Authored-By: Jordan Harband <ljharb@gmail.com>
Co-Authored-By: Jordan Harband <ljharb@gmail.com>
Co-Authored-By: Manuel Spigolon <behemoth89@gmail.com>
| - Does the package have tests and do they run automatically in a CI system, e.g. Travis or GitHub Actions. | ||
| - Is the package actively maintained? Lack of activity in the source control does not mean it is unmaintained, as it may simply be done, but it is something to pay attention to. | ||
| - Make sure the package is not already marked as deprecated on npm and in the README - authors may give you suggestions for alternatives. | ||
| - Does the project offer support? Do the releases have a changelog to ease the update? |
There was a problem hiding this comment.
Just a note for later, this might be a place to reference the support info (once we start to see adoption) as one way to figure this out.
| - [Greenkeeper](https://greenkeeper.io/) | ||
| - Creates branches to test even the in-range updates and reports status if they make tests fail |
There was a problem hiding this comment.
FWIW Greenkeeper is being sunset on 3rd June and are no longer allowing new signups.
There was a problem hiding this comment.
Thanks, I'll update this.
There was a problem hiding this comment.
it's being replaced with a Snyk service, however, so we'll probably want to mention that instead.
There was a problem hiding this comment.
Snyk is already mentioned below below
There was a problem hiding this comment.
I'll remove the Greenkeeper section, but I'm quite concerned about this bit:
It does not require or directly engage with a CI tool.
Which basically means, that there is no more tool, which can run tests with new deps, and raise alerts that your CI won't pass even with an in-range update?
I'm sure someone will come along and create such a service, but for now - even though my personal preference is not GK - that feels like a major loss of functionality?
There was a problem hiding this comment.
I was concerned about that tool, as a dep updating tool that doesn't run CI is basically useless.
The impression folks at Snyk gave me was that this was an error in greenkeeper's post about the topic, and that it would, just like greenkeeper, invoke your CI - but it'd be good to get explicit confirmation.
There was a problem hiding this comment.
@lirantal - do you think you could suggest a new wording for post-greenkeeper snyk please?
There was a problem hiding this comment.
Remove the whole section.
|
|
||
| All of the below tools provide a GitHub App to create PRs with updates. They also support maintaining lock files to update sub-dependencies. Each tool also has built-in or community supported ways of automatically merging the PRs as long as tests pass, although this does carry some risk. | ||
|
|
||
| - [Dependabot](https://dependabot.com/) |
There was a problem hiding this comment.
This was acquired by github, as greenkeeper was by snyk, some consolidation is occuring.
|
|
||
| The `package-lock.json` will get created automatically by default and it will not be published into the registry, i.e. it is only intended to be used for development purposes. npm recommends you commit this file into your source control. | ||
|
|
||
| You can chose to use the `npm shrinkwrap` command to instead create an `npm-shrinkwrap.json` file. It is the same in structure as the `package-lock.json`, but it will also be published together with your package in the registry, which means that not only people who develop the package, but also the users will receive the exact set of dependencies. |
There was a problem hiding this comment.
| You can chose to use the `npm shrinkwrap` command to instead create an `npm-shrinkwrap.json` file. It is the same in structure as the `package-lock.json`, but it will also be published together with your package in the registry, which means that not only people who develop the package, but also the users will receive the exact set of dependencies. | |
| You can choose to use the `npm shrinkwrap` command to instead create an `npm-shrinkwrap.json` file. It is the same in structure as the `package-lock.json`, but it will also be published together with your package in the registry, which means that not only people who develop the package, but also the users will receive the exact set of dependencies, and will be unable to dedupe transitive dependencies with the rest of their dependency graph. |
There was a problem hiding this comment.
A note on this is already included under the downsides?
There was a problem hiding this comment.
I don't think we can over-warn against including a shrinkwrap file in packages.
There was a problem hiding this comment.
i'd still like this suggestion accepted :-)
There was a problem hiding this comment.
In the spirit of #373 does anyone have any objections to having this just be in docs/, as opposed to be a draft in docs/drafts/?
Also, do we want to add it the table of contents?
https://github.com/nodejs/package-maintenance/tree/master/docs#table-of-contents
|
|
||
| ### Defining the dependency version ranges | ||
|
|
||
| JavaScript ecosystem relies on [semantic versioning](https://semver.org/) and most packages respect its rules. Therefore, it is usually recommended that your `package.json` accepts new minor/patch releases of your dependencies automatically. This is the default behavior of npm - when you first `npm install [new dependency]`, it will by default prefix the version with a `^` to indicate that all new releases under the same major version are acceptable. |
There was a problem hiding this comment.
| JavaScript ecosystem relies on [semantic versioning](https://semver.org/) and most packages respect its rules. Therefore, it is usually recommended that your `package.json` accepts new minor/patch releases of your dependencies automatically. This is the default behavior of npm - when you first `npm install [new dependency]`, it will by default prefix the version with a `^` to indicate that all new releases under the same major version are acceptable. | |
| The JavaScript ecosystem relies on [semantic versioning](https://semver.org/) and most packages respect its rules. Therefore, it is usually recommended that your `package.json` accepts new minor/patch releases of your dependencies automatically. This is the default behavior of npm - when you first `npm install [new dependency]`, it will by default prefix the version with a `^` to indicate that all new releases under the same major version are acceptable. |
|
|
||
| ### Using lock files | ||
|
|
||
| The lock files are a snapshot of your full dependency tree, incl. all sub-dependencies, their precise versions and also integrity hashes. Having a lock file allows npm to recreate the dependency tree exactly as it was. npm supports two lock files: `package-lock.json` and `npm-shrinkwrap.json`. Yarn uses its own lock file format. |
There was a problem hiding this comment.
| The lock files are a snapshot of your full dependency tree, incl. all sub-dependencies, their precise versions and also integrity hashes. Having a lock file allows npm to recreate the dependency tree exactly as it was. npm supports two lock files: `package-lock.json` and `npm-shrinkwrap.json`. Yarn uses its own lock file format. | |
| Lockfiles are a snapshot of your full dependency tree, incl. all sub-dependencies, their precise versions and also integrity hashes. Having a lockfile allows npm to recreate the dependency tree exactly as it was. npm supports three lockfiles: `package-lock.json` and `npm-shrinkwrap.json`, and in npm 7+, `yarn.lock`, Yarn's format. |
| - npm documentation: [About semantic version](https://docs.npmjs.com/about-semantic-versioning) | ||
| - npm documentation: [Updating packages downloaded from the registry](https://docs.npmjs.com/updating-packages-downloaded-from-the-registry) | ||
|
|
||
| ### Using lock files |
There was a problem hiding this comment.
| ### Using lock files | |
| ### Using lockfiles |
|
|
||
| The lock files are a snapshot of your full dependency tree, incl. all sub-dependencies, their precise versions and also integrity hashes. Having a lock file allows npm to recreate the dependency tree exactly as it was. npm supports two lock files: `package-lock.json` and `npm-shrinkwrap.json`. Yarn uses its own lock file format. | ||
|
|
||
| The `package-lock.json` will get created automatically by default and it will not be published into the registry, i.e. it is only intended to be used for development purposes. npm recommends you commit this file into your source control. |
There was a problem hiding this comment.
| The `package-lock.json` will get created automatically by default and it will not be published into the registry, i.e. it is only intended to be used for development purposes. npm recommends you commit this file into your source control. | |
| `package-lock.json` will get created automatically by default and it will not be published into the registry, i.e. it is only intended to be used for development purposes. npm recommends you commit this file into your source control. |
|
|
||
| The `package-lock.json` will get created automatically by default and it will not be published into the registry, i.e. it is only intended to be used for development purposes. npm recommends you commit this file into your source control. | ||
|
|
||
| You can chose to use the `npm shrinkwrap` command to instead create an `npm-shrinkwrap.json` file. It is the same in structure as the `package-lock.json`, but it will also be published together with your package in the registry, which means that not only people who develop the package, but also the users will receive the exact set of dependencies. |
There was a problem hiding this comment.
i'd still like this suggestion accepted :-)
|
|
||
| You can chose to use the `npm shrinkwrap` command to instead create an `npm-shrinkwrap.json` file. It is the same in structure as the `package-lock.json`, but it will also be published together with your package in the registry, which means that not only people who develop the package, but also the users will receive the exact set of dependencies. | ||
|
|
||
| Using the lock files has downsides: |
There was a problem hiding this comment.
| Using the lock files has downsides: | |
| Using lockfiles has downsides: |
|
|
||
| - `package-lock.json` (and `yarn.lock`) is only used in development, so you may be testing your package with a different set of dependency versions than your users will be using it, which may occasionally lead to unexpected results. | ||
| - no dependency updates will be received automatically with `npm install`. | ||
| - using a shrinkwrap in a package intended to be used as a library may prevent deduplication of dependencies, increasing the `node_modules` / bundle size. |
There was a problem hiding this comment.
| - using a shrinkwrap in a package intended to be used as a library may prevent deduplication of dependencies, increasing the `node_modules` / bundle size. | |
| - using `npm-shrinkwrap.json` in a package intended to be used as a library may prevent deduplication of dependencies, increasing `node_modules` / bundle size. |
| - `package-lock.json` is only used in development, so you may be testing your package with a different set of dependency versions than your users will be using it, which may occasionally lead to unexpected results. | ||
| - no dependency updates will be received automatically with `npm install`. | ||
|
|
||
| You can disable the lock files in your global or local `.npmrc` file by adding a `package-lock=false` line. This does however mean that npm will also ignore the shrinkwraps included in your dependencies. |
There was a problem hiding this comment.
| You can disable the lock files in your global or local `.npmrc` file by adding a `package-lock=false` line. This does however mean that npm will also ignore the shrinkwraps included in your dependencies. | |
| You can disable lockfiles in your global or local `.npmrc` file by adding a `package-lock=false` line. This does however mean that npm will also ignore the `npm-shrinkwrap.json` files included in your dependencies. |
| - [Greenkeeper](https://greenkeeper.io/) | ||
| - Creates branches to test even the in-range updates and reports status if they make tests fail |
|
Looks like at some point I decided to delete my fork and so now this is showing "unknown repository" as source. I restored the |
dominykas
added
the
package-maintenance-agenda
|
@dominykas |
Sure, but can we do it after everything is merged, because #302 (comment) and #425? |
dominykas
removed
the
package-maintenance-agenda
Closes #280.
I feel this is too long, but right now I'm not sure what can I leave out... There's probably some stuff which should just point to some external content.
Anything that's worth expanding on? Did I miss anything important?