Skip to content

Loading…

Write installed modules into package.json #2999

Closed
wants to merge 27 commits into from
@lloydwatkin

I've hit the issue a couple of times where I develop code, and then forget to npm i <package> --save as I go along. So what I've done here is write a small feature which reads all the package.json files in node_modules (first level only), extracts the version number and writes the results to package.json.

If you provide 'insert' after the command then existing dependencies list is not clobbered but any packages already in package.json have their version number overwritten.

I did intend to extend by allowing a user to, if desired, to choose whether the dependency was dev or standard but I'm tight for time at present.

If this is something that gets accepted then I'll possibly come back and add later.

I added docs in doc/cli/backfill.md but if more are required please let me know where.

@nathan7
npm member

..please, squash this into one commit.

@lloydwatkin

Got a response from Tobias that doesn't seem to be appearing here, it said:

what is the different to npm shrinkwrap?

With shrinkwrap you need your dependencies in package.json already in order to generate npm-shrinkwrap.json. In this case you generate that dependency list from what is installed already.

@mfncooper
npm member

I've hit the issue a couple of times where I develop code, and then forget to npm i --save as I go along.

Wouldn't it be simpler to just have this in your .bash_profile or wherever?

export npm_config_save=true

I don't really see a need for a whole new command, given the stated use case.

@nathan7
npm member

There are also some serious issues with this code (taking the return value from an async function? what the hell?) and it doesn't conform to npm's style.

@isaacs
npm member

@mfncooper: Yes, you can also do npm config set save true, but then that can cause weird things when you don't want to save. (Though, now that npm rm removes stuff from package.json when --save is set, maybe it'd all be good... I don't know. Might be worth trying out for a while :) Also, you might npm install some stuff before you've got a package.json file, if you're just at the "poking around" stage.

@lloydwatkin

Thanks! Overall, I think this feature is generally a good idea. Here's some things I'd like to see.

From a functionality point of view, npm backfill insert is weird. It doesn't really match the format of any other npm command. Positional arguments are either the verb ("backfill" in this case) or nouns (ie, package identifiers, usually). Adjectives and adverbs are --flags, which are defined as configuration options.

Maybe npm backfill [<packages> ...] could just put the specified packages into package.json. That'd match the behavior of ls, outdated, and update, which only do their thing on the specified package name. It seems like the "insert" behavior is pretty much always what you'd want, so I'd just make that "how it works" rather than a special flag. How does that sound to you? Do you have a use case for the non-insert style?

Regarding the implementation, I have two nits:

  1. No sync I/O allowed. There's a module at require("read-package-json") that you can use to load in the package.json file. You pass it a path and a callback, and it calls the callback with an error if there was one. (This will also read the package comment from an index.js file, fill in defaults, etc.)
  2. Style. I am not too much of a stickler for minimal semicolon usage in pull reqs, since I can just fix that up myself easily enough, but the indents are inconsistent and that makes this harder to read. This is the most minor point. In fact, you can ignore it if you like, and I'll just do it for you.

If you feel like you've done enough and don't feel like working any more on this, I totally understand. If you leave it here, I'm sure someone will eventually come along (maybe me or @mfncooper or @nathan7) and massage it into shape.

@lloydwatkin

Hi @isaacs that's great feedback thanks.

Firstly Wuthering regards to formatting looks like my stupid IDE has put in stupid tabs despite being told not too, damn thing :)

With regards to 'insert', I guess I'd like a list of exactly what I have installed at that time. Maybe we could make this a --clean flag instead and have 'insert' as default?

I'd like to push through on this if that's ok? I'm new to node and the async paradigm so it would be great feedback on updates (provided that's ok with you guys?). I never expected this to be final code, more to start the conversation and should have pointed this out, my apologies.

@lloydwatkin

Hi guys,

Updated commit with consistent indentation and totally async.

The code now works as suggested i.e. dependencies list is not clobbered (although I've made this a flag --clean). If you provide a package name then only this package is updated (you can provide multiple package names), if you provide none then all are updated.

Feedback appreciated and happy to edit again if required.

Cheers, Lloyd.

@isaacs isaacs commented on an outdated diff
lib/backfill.js
((34 lines not shown))
+ packageConfig.devDependencies = {};
+ }
+ async.map(list, updateDependencies, function(error, results) {
+ if (error) return cb(error);
+ fs.writeFile(jsonFile, JSON.stringify(packageConfig, null, 2), function(error) {
+ if (error) return cb(error, null);
+ cb(null, null);
+ });
+ });
+ });
+ });
+}
+
+updateDependencies = function(package, callback) {
+ readJson(path.resolve('./node_modules/' + package, "package.json"), function(error, json) {
+ if (error) return callback(error);
@isaacs npm member
isaacs added a note

This will fail if any packages are installed that have bins, because it'll try to read node_modules/.bin/package.json, which never exists. It would be better to just skip over any non-package things here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@isaacs isaacs commented on an outdated diff
lib/backfill.js
((12 lines not shown))
+ + "\nnpm backfill --clean <package-name>"
+ + "\nIf --clean flag is supplied any existing [dev]Dependencies are clobbered"
+
+backfill.completion = function (opts, cb) { }
+
+var path = require("path")
+ , fs = require("graceful-fs")
+ , npm = require("./npm.js")
+ , semver = require("semver")
+ , async = require('async')
+ , readJson = require("read-package-json");
+
+function backfill(args, silent, cb) {
+
+ if (typeof cb !== "function") cb = silent, silent = false;
+ jsonFile = path.resolve('.') + '/package.json';
@isaacs npm member
isaacs added a note

Use npm.prefix here, not .. You don't know that it's being called in the package root. Also: slashes.

jsonFile = path.resolve(npm.prefix, "package.json")
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@isaacs isaacs commented on an outdated diff
lib/backfill.js
((33 lines not shown))
+ packageConfig.dependencies = {};
+ packageConfig.devDependencies = {};
+ }
+ async.map(list, updateDependencies, function(error, results) {
+ if (error) return cb(error);
+ fs.writeFile(jsonFile, JSON.stringify(packageConfig, null, 2), function(error) {
+ if (error) return cb(error, null);
+ cb(null, null);
+ });
+ });
+ });
+ });
+}
+
+updateDependencies = function(package, callback) {
+ readJson(path.resolve('./node_modules/' + package, "package.json"), function(error, json) {
@isaacs npm member
isaacs added a note

path.resolve(npm.dir, package, "package.json")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@isaacs
npm member

There are still (at least) 2 problems here:

  1. Adding the async module is not going to happen. You can use require('slide').asyncMap which looks like it does roughly the same thing as async.map. If we were to move to async, it would have to be done consistently across the entire npm codebase, and that's outside the scope of this pull request.
  2. Style: 2-space indents. Double-quotes. Minimal semicolons. Please see the style guide

From a more substantive point of view:

I think it's a bit odd to have the --clean behavior bundled in with this. If something is called "backfill", you'd assume that it's just filling in gaps, not removing extraneous things.

Since this is essentially a fill-in for having forgotten to specify --save previously, maybe it should work more like npm install --save does. That is, put a ~ char in front of the effective version, save to devDependencies if --save-dev is set, optionalDependencies if --save-optional is set, and add the name to bundledDependencies if --save-bundle is set. What do you think?

@lloydwatkin

Hi Isaacs,

Thanks for the feedback, will get code updated as soon as I get a chance.

As for the --clean functionality the use case I've been using is where I've been developing some code and adding/removing packages a will and wanting to get a clean representation of what is actually there. How does that use case work for you?

Happy to add the --save-* flags as well, it makes sense. Originally I had planned something like this but didn't code up.

@guybrush

I really like this feature!

Though i cant see how this works with packages installed via git-urls. Which actually doesn't matter that much, just wanted to mention.

@lloydwatkin

Hey @isaacs all requested changes made, I've left clean in there as there was no response to my use-case, if its really a stumbling block to merging then I will remove. I personally feel it is useful, but I'm not everybody :)

Lloyd

@mfncooper
npm member

Honestly, this seems over-complicated to me. Why not just have npm sync that updates package.json to match (i.e. be in sync with) whatever is in node_modules? If there are packages that need adding, it'll add them; if there are packages that aren't installed, it'll remove them. If you want to get fancier than just syncing, just edit package.json. It's not hard. :) No need for insert or clean or anything. No need to worry about the user providing a list of packages that may or may not be installed, and whether we should be installing those or not.

@isaacs: The advantage to setting the env var versus setting the config var is that I can set it when I start working on a project, and it's set only in that shell, and is "reset", so to speak, when I exit that shell. So it's a lot more likely that I'm only going to have it set only when I want it set. In contrast, the config var would apply in all shells, for all projects.

@isaacs
npm member

Yeah, just syncing up with the contents of node_modules is probably best.

In the latest version, you can see if it was a git url etc by the _from and _resolved fields, which is used by --save and shrinkwrap, respectively.

@mfncooper (off-topic) Ah, I see. That's true. My shells tend to be very short-lived, what with tabs and vim :sh and split screens and such. When you say "set an env", I just assumed you meant "set it .bashrc" or something.

@lloydwatkin

@mfncooper if you refer back to my original use case then this is exactly what I was trying to achieve:

I've installed/removed a load of packages whilst devleoping and haven't done npm ---save or had env
variable set and I'd like to get my package.json to match what I have

At present the sync functionality would be covered by doing npm backfill --clean. Maybe it would be worth backtracking somewhat and renaming the method to sync and making --clean the default behaviour (this basically then gets us to what you were suggesting)?

I think checking for a git URL and using this in preference to a version number (provided these are only set when module comes directly from git?) would be great.

Leaving the other functionality in place is probably ok, because its not used unless you decide to look into it and start using them. Also if I had done a full sync followed by adding some devDependencies then the more advanced user could choose to only sync that one module to the correct place.

How does that sound guys?

@isaacs
npm member

@lloydwatkin I think that sounds great. Much simpler, and more intuitively obvious what it does. Thanks for hanging in there. I think there's a good feature in here, we're going to get to it eventually :)

To recap:

  1. Rename your "backfill" to "sync".
  2. If there are positional args, then only synchronize the specified package names.
  3. Should behave exactly like npm install --save, as if you'd just installed whatever is there. (Or npm rm --save if the package is not installed.)
  4. If --save-dev or --dev is set, then sync any deps to devDependencies that are not already in dependencies.
  5. If --save-optional is set, then sync any deps to optionalDependencies that are not already in dependencies.
  6. Remove packages from dependencies, devDependencies, or optionalDependencies that are not installed.
@lloydwatkin

Hey guys,

Sorry for the long wait on this, I've been very busy with other projects.

Code has been updated to match that agreed in @isaacs last message. Its working rather nicely and covers the original use case that I was looking to fulfil.


There's only a single issue that I can see which is as follows:

  • I sync a package to my devDependencies
  • I sync all packages
  • Package is now also registered in my dependencies also

Ideally it should only add packages that aren't installed elsewhere, although maybe I'm worrying too much about the fine details now rather than the overall goal of the command.

Happy to add if it is felt that this should be how it should behave. Grateful for any feedback as always.

@Bartvds

Is this still considered for merging? This functionality would be useful when consolidating prototypes and experiments.

@doismellburning

As someone currently writing his first Node app having come from Python and being used to the model of:

$ pip install foo
$ pip install bar
$ pip freeze > requirements.txt

so didn't actually consider the existence of the --save option to install; this would have solved my (admittedly not huge, but mildly inconvenient nonetheless) problem of a few minutes ago - I'd love to see it merged in at some point!

@domenic
npm member

Hmm don't we already have this with npm init?

@luk-
@doismellburning

Yep, so I just deleted the one I'd already made as a workaround...

@Bartvds

@domenic To simulate the use case open an existing project and remove some of the dependency items from the package.json as if you'd forgotten to use --save when installing.

Now what npm command can we use to automatically restore those?

I tried npm init and it doesn't do that at all.

@rscheuermann

Hi guys, new to npm, and I'm just noting that I ran into a need for the sync command today, and I'd love to see this make it's way in!

Anything I can do to help it along?

@Jxck

+1

@watilde

Good idea:)

@sholladay

+1

I wish for this constantly. npm generally makes experimenting very easy, but sometimes I run into bugs when moving beyond that stage, because this process is currently very manual.

@othiym23

I'm genuinely really sorry, @lloydwatkin, but this patch has now drifted so far from the current code base that I don't think it's possible to clean it up and merge it as-is. I also think that the requirements for the feature have become a little clearer with some of the changes to npm@3, especially given the way that npm@3 handles shrinkwrap files more transparently / in a more integrated fashion. Therefore, I'm closing this PR in favor of #9230 (which is also part of the official npm road map). If you wanted to take another stab at this, in light of what's in #9230, I bet you could probably team up with @iarna to make it a pretty painless effort to split up the work she's talked about doing to support npm init. Thanks for your patience and contribution!

@othiym23 othiym23 closed this
@lloydwatkin

@othiym23 hey no problem, these things happen. It looks like something similar may be coming along very shortly which will be great. I'm not sure I've got time at present but if some becomes available I'll certianly get in touch with @iarna. Thanks for all your efforts on npm its a great tool!

@lloydwatkin lloydwatkin deleted the lloydwatkin:backfill branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 14, 2012
  1. @lloydwatkin

    Add backfill feature

    lloydwatkin committed
Commits on Dec 18, 2012
  1. @lloydwatkin

    Add async module

    lloydwatkin committed
  2. @lloydwatkin
  3. @isaacs @lloydwatkin

    node-gyp@0.8.1

    isaacs committed with lloydwatkin
    Add backfill feature
  4. @lloydwatkin
  5. @lloydwatkin
Commits on Jan 21, 2013
  1. @lloydwatkin

    Skip over non-package things

    lloydwatkin committed
  2. @lloydwatkin

    Use npm.prefix not '.'

    lloydwatkin committed
  3. @lloydwatkin

    Use resolve properly

    lloydwatkin committed
  4. @lloydwatkin

    2-space indent

    lloydwatkin committed
  5. @lloydwatkin
  6. @lloydwatkin

    'async'-out --> 'slide'-in

    lloydwatkin committed
  7. @lloydwatkin
  8. @lloydwatkin

    Removed async dependency

    lloydwatkin committed
  9. @lloydwatkin
  10. @lloydwatkin
  11. @lloydwatkin
  12. @lloydwatkin
  13. @lloydwatkin

    Removed trailing whitespace

    lloydwatkin committed
  14. @lloydwatkin

    Update docs to match

    lloydwatkin committed
  15. @lloydwatkin
Commits on Feb 16, 2013
  1. @lloydwatkin

    Rename backfill -> sync

    lloydwatkin committed
  2. @lloydwatkin
  3. @lloydwatkin
  4. @lloydwatkin

    Reworked allPackages loading

    lloydwatkin committed
  5. @lloydwatkin

    Update documentation

    lloydwatkin committed
  6. @lloydwatkin

    Minor change to docs

    lloydwatkin committed
This page is out of date. Refresh to see the latest.
Showing with 148 additions and 2 deletions.
  1. +1 −0 .gitignore
  2. +44 −0 doc/cli/sync.md
  3. +3 −2 lib/npm.js
  4. +100 −0 lib/sync.js
View
1 .gitignore
@@ -16,3 +16,4 @@ npm-debug.log
/npmrc
/release/
/npm-*.tgz
+.project
View
44 doc/cli/sync.md
@@ -0,0 +1,44 @@
+npm-sync(1) -- Add installed modules to package.json
+========================================================
+
+## SYNOPSIS
+
+ npm sync
+ npm sync <package-name> [ <package-name1> ... <package-nameN> ]
+ npm sync [--save|--save-dev|--save-optional|--save-bundle] [<package-name> ... ]
+
+
+## DESCRIPTION
+
+If you've installed a new package and forgotten to do `npm i --save <package-name>`
+then this command will allow you to sync packages into your packages.json file:
+
+```
+ npm sync <package-name>
+```
+
+You can also sync several packages at once by doing:
+
+```
+ npm sync <package-name1> <package-name2> .... <package-nameN>
+```
+
+Additionally by not providing any arguments you can sync all currently installed
+packages:
+
+```
+ npm sync
+```
+
+ Note: This will remove any packages which are not installed from your `package.json`.
+
+`npm sync` takes 4 exclusive, optional flags which save or update the package version in
+your main package.json (respected in the same order):
+
+ * `--save`: Package will appear in your `dependencies`
+ * `--save-dev`: Package will appear in your `devDependencies`
+ * `--save-optional`: Package will appear in your `optionalDependencies`
+ * `--save-bundle`: Package will appear in your `bundleDependencies`
+
+ Note: If package exists in `dependencies` but you are syncing a package to
+ another type of dependency it will not be added
View
5 lib/npm.js
@@ -121,10 +121,11 @@ var commandCache = {}
, "submodule"
, "pack"
, "dedupe"
-
+
, "rebuild"
, "link"
-
+ , "sync"
+
, "publish"
, "star"
, "tag"
View
100 lib/sync.js
@@ -0,0 +1,100 @@
+/*
+ * npm sync [insert]
+ *
+ * See doc/sync.md for more description
+ */
+
+module.exports = sync
+
+sync.usage = "npm sync"
+ + "\nnpm sync --save-dev"
+ + "\nnpm sync --save-bundle"
+ + "\nnpm sync --save-optional"
+ + "\nnpm sync <package-name> [... <package-name2>]"
+ + "\nnpm sync <package-name>"
+
+sync.completion = function (opts, cb) { }
+
+var path = require("path")
+ , fs = require("graceful-fs")
+ , npm = require("./npm.js")
+ , semver = require("semver")
+ , slide = require("slide")
+ , readJson = require("read-package-json")
+
+var validKeys = {
+ "save": "dependencies",
+ "dev": "devDependencies",
+ "save-dev": "devDependencies",
+ "save-optional": "optionalDependencies",
+ "save-bundle": "bundleDependencies"
+}
+
+function sync(args, silent, cb) {
+ if (typeof cb !== "function") cb = silent, silent = false
+ callback = cb
+ syncPackagesCount = args.length
+ jsonFile = path.resolve(npm.prefix, "package.json")
+ readJson(jsonFile, function(error, config) {
+ if (error) return cb(error, null)
+ packageConfig = config
+ if (0 == syncPackagesCount) //{
+ allPackages = Object.keys(packageConfig.dependencies || {}).concat(
+ Object.keys(packageConfig.optionalDependencies || {}),
+ Object.keys(packageConfig.devDependencies || {})
+ )
+ getInstalledPackageList(args, updatePackageList)
+ })
+}
+
+var updatePackageList = function(error, list) {
+ var writePackageFile = function(error, results) {
+ if (error) return cb(error)
+ fs.writeFile(jsonFile, JSON.stringify(packageConfig, null, 2), function(error) {
+ callback(error, null)
+ })
+ }
+ if (error) throw Error(error)
+ dependencyKey = getDependencyKey()
+ setupDependencyList(dependencyKey, arguments)
+ if (allPackages)
+ allPackages.filter(function(i) {return !(list.indexOf(i) > -1);}).forEach(function(package) {
+ list.push(package)
+ })
+ slide.asyncMap(list, updateDependencies, writePackageFile)
+}
+
+var setupDependencyList = function(key, args) {
+ if (!packageConfig[key] || (0 == syncPackagesCount))
+ packageConfig[key] = (key == "bundleDependencies") ? new Array() : {}
+}
+
+var getDependencyKey = function() {
+ for (var key in validKeys)
+ if (npm.config.get(key)) return validKeys[key]
+ return "dependencies"
+}
+
+var updateDependencies = function(package, callback) {
+ readJson(path.resolve(npm.dir, package, "package.json"), function(error, json) {
+ if (error) {
+ if ('ENOENT' == error.code) {
+ delete (packageConfig['dependencies'] || {})[package]
+ delete (packageConfig['devDependencies'] || {})[package]
+ delete (packageConfig['optionalDependencies'] || {})[package]
+ }
+ } else if (dependencyKey == "bundleDependencies") {
+ if (!packageConfig['dependencies'][package])
+ packageConfig[dependencyKey].push(package)
+ } else {
+ if (dependencyKey == 'dependencies' || !packageConfig['dependencies'][package])
+ packageConfig[dependencyKey][package] = "~" + json.version
+ }
+ callback(null, true)
+ })
+}
+
+var getInstalledPackageList = function(args, callback) {
+ if (args.length > 0) return callback(null, args)
+ fs.readdir(path.resolve(npm.dir), callback)
+}
Something went wrong with that request. Please try again.