Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update command #22

Open
dustinboston opened this issue Apr 18, 2012 · 13 comments
Open

Update command #22

dustinboston opened this issue Apr 18, 2012 · 13 comments

Comments

@dustinboston
Copy link

I was wondering if an update command was in the mix. It would be sweet if you could just do a "volo update" and get all the latest version of the dependencies listed in the package.json file.

@dustinboston
Copy link
Author

Scratch that, I did it! I would fork and submit a pull request but I'm not very familiar with the code base OR node yet. In fact, this is my first time using node altogether. :-)

It appears to be working with my little test case and I think it would be a great core command. Let me know what you think.

'use strict';
/*jslint */
/*global define */

define(function (require, exports, module) {
    var fs = require('fs'),
        path = require('path'),
        packageJson = require('volo/packageJson'),
        add = require('add'),
        main;

    main = {
        //Text summary used when listing commands.
        summary: 'Update all project dependencies',

        //Longer doc text used with volo.js help
        //The 'text' module is available as part
        //of volo.js
        doc: require('text!./doc.md'),

        //Validate any arguments here.
        validate: function (namedArgs) {
            //Return an error to indicate a validation problem.
            //If everything is OK, do not return a value.
            //return new Error('There was a validation error.');
        },

        run: function (d, v, namedArgs) {
            var success = true,
                pkg = packageJson('.'),
                deps,
                dep;

            if (pkg && pkg.data && pkg.data.volo && pkg.data.volo.dependencies) {
                deps = pkg.data.volo.dependencies;
                for (dep in deps) {
                    add.run.apply(add, [d, v, namedArgs, dep]);
                }
            }

            if (success) {
                d.resolve();
            } else {
                d.reject(new Error('There was an error.'));
            }
        }
    };

    return require('volo/commands').register(module.id, main);
});

@jrburke
Copy link
Member

jrburke commented Apr 22, 2012

Sweet, thanks for taking a shot at this. This is looking good. I think you may need to force the -f flag for add by setting namedArgs.force = true, otherwise run may just warn that the item is already installed.

So this gets into more of what the uses cases are:

  1. You pull down a project and it does not have any of the dependencies installed, just items listed in the package.json.

The above approach works for that. Although I would tend to want to just use the code you have for the volo.dependencies iteration and do that as part of the "add" command. Maybe if no dependency is listed, just a volo add, then this behavior is done.

  1. You already have dependencies on disk and you want to update them.

This case would likely need the the -f option. Although at that point, we could just have that be volo add -f.

What do you think?

The other thing: right now add will try to add the version tag into the volo.dependencies, so for 'jquery' it may show up as 'github:jquery/jquery/1.7.2`. So doing an update on that info is not very useful, at least for case 2, just useful for case 1.

If you specified a branch though it would be helpful, so 'jquery/jquery/master' for example.

So if that matches your expectations, maybe we can just roll that into add, unless you think it is clearer to have a specific command for it.

@dustinboston
Copy link
Author

I think you're right. It would be much simpler to merge the update functionality into the add command. Really the only argument for an update command is clarity. Wonder if volo update could simply be an alias to volo add?

@jrburke
Copy link
Member

jrburke commented Apr 23, 2012

I can see that. Need to look into allowing command aliases. Although, i expect an update command needs to have a good set of docs of what gets updated -- specifically that jquery/jquery/1.7.2 may not do anything if jquery is already installed at that version, but jquery/jquery/master might trigger an update.

So maybe it is a shell command that mostly proxies to add, but has its own doc set and such.

So out of curiousity, any thoughts on the 1) and 2) scenarios above? Which ones would you use/were you thinking of when opening this ticket?

@dustinboston
Copy link
Author

My use case has been number 2: the need to update existing packages. The main project I'm working on right now has a lot of third party dependencies :-( and I hate checking each one manually, then copying the files down, checking them in etc. The unfortunate thing is that these deps are in a libs folder (not a js folder) so I still have to move them into the correct location after I run volo update.

Regarding number 1: It would be pretty badass to have a project with marked dependencies that I did not have to distribute with the project. This is when volo add would be perfect. Essentially fetching all un-installed deps.

Just had a thought regarding those version tags. If I run volo add -f jquery, I want the same behavior as when I run volo add jquery and do not have it already downloaded. In other words, it should go out, do a search, and grab a new versioned file. The intelligent part of the process would be to update the volo.dependencies list to have the new tag for jquery. For now I would suggest leaving the old files laying around. In the future there could be a volo cleanup which would remove old versions that are not listed in volo.dependencies.

@mstade
Copy link
Contributor

mstade commented May 15, 2012

I've created a separate command for resolving dependencies, that just calls out to volo add -f <dependency>. This gives a subtle benefit of being able to exclude dependencies we don't want resolved.

Resolving dependencies (case 1 & 2): volo resolve
Resolving dependenciew while excluding foo and bar: volo resolve foo bar

The last call reads rather badly (I always think it means resolving those dependencies exclusively!) This could all be rolled into add, with exclusions being a named parameter if only I could figure out how to actually do that :)

The semantics of the verb add starts to muddy a bit though, just like @dustinboston mentioned.

@mstade
Copy link
Contributor

mstade commented May 15, 2012

Also worth noting, making multiple parallel calls to volo add will most likely fail (at least on windows.) I have a nagging feeling that this is because add keeps some state (temp files?) that then makes it confused. I had to implement a sort of queue mechanism that essentially turns this:

["foo", "bar", "baz"].forEach(function(dependency) {
    v.command("add", dependency, "-f")
})

Into:

["foo", "bar", "baz"].forEach(function(dependency) {
    queue.add(function() {
        return v.command("add", dependency, "-f")
    })
})

queue.then(d.resolve)

Maybe q already supports this type of scenario, but I'm too stupid to figure it out from the docs :)

Queue would of course not be necessary if add had support for running in parallel and/or accepted a list of dependencies instead of a single one. That's kinda nice actually, something like:

volo add would download all dependencies declared in package.json

volo add jquery would add jquery to package.json and then download it

volo add jquery backbone would add jquery and backbone to package.json and then download both

@jrburke
Copy link
Member

jrburke commented May 15, 2012

@mstade: sorry there is not enough docs to help make commands right now. I have seen your other issues you logged, and hoped to resolve them by actually putting up some docs. I think the main thing is the need to wire up the fail/rejected promises to any promise chain so that errors bubble up correctly.

In the latest 0.1.5 code there is now a v.sequence() that takes an array of args, and passes each arg array to spawn() and calls them in sequence, waiting for the first to finish before firing the next one. Example:

https://github.com/volojs/volo/blob/master/vololib/volo/v.js#L193

Here is a longer example:

https://github.com/volojs/create-responsive-template/blob/master/volofile#L313

Also, I wonder if the exclusion list can just be handled via a namedArg, like:

volo update exclude=foo,bar

Quick status update on this: I have been doing some plumbing work, and I'm currently looking at allowing volo commands to specify dependencies, and to allow using npm-installed modules, like jshint locally in a project's volofile. Once I have that sorted out, then I'll be looking more at the update command. I have been doing some 0.1.x point releases since some people at work have been using volo, and wanted to unblock their work.

An update command is still in the plans for a 0.2 release. Keep the comments and design work coming in this ticket.

@mstade
Copy link
Contributor

mstade commented May 16, 2012

I'd be happy to help you out with this, I'm feeling more and more confident about volo and wouldn't mind contributing at all. I will be scanning and e-mailing my CLA to the dojo foundation tonight, and will have time this weekend to work.

You're absolutely right about using a named arg for exclusions, it makes the most sense. However as mentioned, I just don't know how to use them yet :)

Being able to use npm installed modules would be great. Right now I'm fully qualifying the paths to the npm installed modules I need--but I'd rather not.

Also, not sure if this is what you mean by allowing volo commands to specify dependencies, but trying to load dependencies that are in v.path won't work unless fully specifying their path. This is not an option however, as those modules may have dependencies that are defined in the usual local format. I.e. let's say my structure is:

/target
    volofile
    /js
        foo.js
        bar.js

If I then try to run require("js/foo") in my volofile, it will bail. In order to fix this, I did this smelly little thing:

require.config({ baseUrl: path.resolve(v.path, "js") })

var foo = require("js/foo") // This now works

require.config({ baseUrl: require("volo/baseUrl") })

It's not pretty, but it works! :)

I know much too little about RequireJS to understand how to fix this in a less smelly way, any info you've got to guide me would be much appreciated!

Your v.sequence implementation is gorgeous btw, seal of approval! Keep up the awesome work dude!

@jrburke
Copy link
Member

jrburke commented Jun 7, 2012

Update on this, there is now recursive add if there is a volo.dependencies in the package.json. No update command yet because I'm not sure what that should look like yet -- maybe it just pulls out the volo.dependencies, pulls off version segments and refetch them with "-f"?

It gets tricky is when there is a recursive fetch. Maybe it is an "add" flag that say "ignore the version number I give you, go get the latest". But at some point you probably do not want to fetch the whole project.

I want to get the new npm/node_modules-friendly codebase out, to see how it holds up because it is a big change, and I do not want to block on most of the previously set 0.2 bugs. I will reschedule them as appropriate. So taking off the 0.2 milestone for this, but I want to keep talking about it.

In any case it should be easier to do a bulk update at least for the top level dependencies by editing the package.json to remove verisons, then doing a volo add -f.

@KidkArolis
Copy link

I'm also wondering, whether volo should have "npm install" style command e.g. "volo add" with no arguments that installs all dependencies as described in package.json. This would allow adding js/lib to .gitignore. I don't know if that is the recommended way to do it, but at least there would be an option to try out this way of doing it. Right now you're kinda forced to check in the dependencies.

And I also agree that volo add could update all libraries that don't have the version locked down to the latest stable version. Perpahs the stable version number could be stored in volojs/repos. (e.g. volo add jquery seems to pull down the master which is not the stable version).

@jrburke
Copy link
Member

jrburke commented Jul 4, 2012

@KidkArolis, volo add with no args should scan the package.json and use those dependency IDs for install:

https://github.com/volojs/volo/blob/master/commands/add.js#L74

but what it does not do right now is offer some flexibility in being able to specify "install the latest 1.7 version".

@KidkArolis
Copy link

Thanks!, I didn't realise. Volo has so many useful things already!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants