Skip to content

Conversation

@joshdmiller
Copy link
Member

Most of the code here (and all of the logic) has been moved to ngbp proper. The ngbp-cli binary is now only responsible for ensuring that ngbp is installed locally and for calling it. All other logic is upstream to prevent needing to update this very frequently.

More information at ngbp/ngbp@40f3a8f.

Most of the code here (and all of the logic) has been moved to ngbp proper. The ngbp-cli binary is
now only responsible for ensuring that ngbp is installed locally and for calling it. All other
logic is upstream to prevent needing to update this very frequently.

More information at 40f3a8f.
@PaulL1
Copy link

PaulL1 commented Jan 5, 2014

Looks good for this portion Josh. I've added a pull request for some minor stylistic updates (pull request #4)

Questions:

  1. Why use deferred for the installation? Are we imagining the installation progressing in the background whilst we do other things? If not, are we essentially using deferred as a way to tidily exit from our branching logic with either success or error?
  2. The implication in the comments is that this won't function without a local ngbp. Does this mean what it seems to - that it wouldn't be happy with a global ngbp? Is that a deliberate design decision - seems like perhaps it could work globally as well - there doesn't appear to be anything about the ngbp functionality that requires it to be local?
  3. Minor, but I think the logic around line 46 says "look for an ngbp script, if there isn't one, look for ngbp.js, if there are neither then return an error". If so, then we're logging an error at line 50 that we'd want to be silent on, or treat perhaps as a log rather than error (i.e. if we're OK with ngbp.js then it's not an error if we don't have ngbp). I haven't looked at the ngbp end of this commit yet, so I'm presuming that ngbp.js is another valid install variant. If that's not the case, then I don't see why the search for ngbp.js is in the catch block

@joshdmiller
Copy link
Member Author

Thanks for taking a look.

Why use deferred for the installation?

Node is inherently asynchronous and we shouldn't shy away from or try to obscure that. Promises make code cleaner, so we don't end up in the Pyramid of Doom (deeply nested callbacks) because it's hard to read. I could have also used caolan/async, but I like this syntax better. Also, the synchronous file operations I included should be made asynchronous again.

Does this mean what it seems to - that it wouldn't be happy with a global ngbp?

Yes. ngbp must be installed locally. First is the reason of best practices; Node modules shouldn't be installed globally in most circumstances. There are a number of challenges to doing so, including with multi-user systems (globals are shared with all users), multi-application developers (applications will depend on different versions of a library), etc. Peer dependencies amplify this problem exponentially, and both ngbp and Grunt make heavy use of peer dependencies.

The second reason is technical; if one is using an ngbp plugin, it will have peer dependencies on ngbp, so NPM is going to install it anyway. For example, if one wants to run coffee, she's going to do npm install ngbp-contrib-coffee, which will install that plugin, it's peer dependency ngbp, and ngbp's peer dependency grunt. While one could, conceivably, make this work through some terminal magic, using wrappers inside ngbp-cli to check for both global and local installations would be problematic and error-prone. And with no upside, per the above paragraph.

The third reason is practical: package.json gives requirements, so when a developer pulls down a repo and runs npm install, it's going to install the packages and all their dependencies locally anyway. There isn't an easy way to avoid local installations even if one wanted to. And again, there's no upside.

Also, for what it's worth, Grunt requires a local installation as well, for the same reasons as above, so we need them anyway. :-)

All that is to say that we don't have a choice: ngbp must be installed locally.

I'm presuming that ngbp.js is another valid install variant. If that's not the case, then I don't see why the search for ngbp.js is in the catch block

It's not another install variant, though it seems that way at first glance. Both scenarios are looking for a local installation, but if we cannot immediately find it, we also want to look in parent directories for it's installation. For example, if you have a project in /projects/my-app and from that directory you run ngbp, it will see inside the node_modules folder that ngbp is installed; but if you run the command from /projects/my-app/src/app, the node_modules folder doesn't exist there and Node cannot find the module. In the latter case, we do the additional check of searching up through the file system looking for lib/ngbp.js. That's the reason for the try/catch. If neither is found, there is no local ngbp installation and this really is a failure from which cannot recover, so we want to install ngbp if permitted by the user.

However, per your point, the error log is wrong; I had it in there when I was debugging and it needs to be removed completely. Good catch!

@joshdmiller joshdmiller merged commit 76f2d49 into master Apr 17, 2014
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

Successfully merging this pull request may close these issues.

3 participants