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

Already on GitHub? Sign in to your account

Added a command-line option to bin/express to generate engine entries for node and npm in package.json #1486

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants

This is a simplified version of a prior pull request based on feedback: visionmedia#1483

@shesek What do you think?

Owner

tj commented Jan 29, 2013

hm.. heroku needs both?

shesek commented Jan 31, 2013

@curious-attempt-bunny Looks great, but perhaps its better to use '~'+process.versions.node rather than hardcoding 0.8.x? I also tried to check how to get npm's version, but it seems like you can't require() it unless you install it locally, so I'm not quite sure how to go about that.

@visionmedia You mean both node and npm versions? I just tried to push to Heroku without an npm version (but with node set to v0.8.x):

-----> Node.js app detected
-----> Resolving engine versions
       Using Node.js version: 0.8.14
       Using npm version: 1.0.106
-----> Fetching Node.js binaries
-----> Vendoring node into slug
-----> Installing dependencies with npm
       Error: npm doesn't work with node v0.8.14
       Required: node@0.4 || 0.5 || 0.6
           at /tmp/node-npm-9SVb/bin/npm-cli.js:57:23
           at Object.<anonymous> (/tmp/node-npm-9SVb/bin/npm-cli.js:77:3)
           at Module._compile (module.js:449:26)
           at Object.Module._extensions..js (module.js:467:10)
           at Module.load (module.js:356:32)
           at Function.Module._load (module.js:312:12)
           at Module.require (module.js:362:17)
           at require (module.js:378:17)
           at Object.<anonymous> (/tmp/node-npm-9SVb/cli.js:2:1)
           at Module._compile (module.js:449:26)

It uses npm v1.0 by default, which only works with node <=v0.6. Seems like it needs both of them.

Owner

tj commented Feb 1, 2013

oh damn, Ideally we just set them without the flag IMHO, I'm not sure people will even notice it's there otherwise, I definitely wouldn't guess it's related to heroku etc, so I would probably just end up adding them manually

@shesek I like sniffing the node version and assuming that. If I go with that do you see a problem with assuming npm 1.1.x?

@visionmedia Sure. I can make it just add the engines by default. Less to document and for the user to have to figure out (unless it would rub people the wrong way).

Okay. This pull request is now much simplified. What do you think? @shesek @visionmedia

shesek commented Feb 19, 2013

Looks fine to me. I think adding it by default makes sense, it shouldn't break anything to anyone.

I'd still like to see this make it's way into master. Currently I have express locally patched. Anything left to improve @visionmedia ?

Owner

tj commented Apr 13, 2013

I dont like the idea of maintaining the npm version separately, maybe we can at least ditch that, my npm is at 1.2.x already I dont want to check up on that all the time

Member

jonathanong commented Sep 10, 2013

this is too strict and would stop people from upgrading to newer versions of node and npm.

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