Skip to content
This repository

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

4 participants

Merlyn TJ Holowaychuk Nadav Ivgi Jonathan Ong
Merlyn

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

@shesek What do you think?

TJ Holowaychuk
Owner

hm.. heroku needs both?

Nadav Ivgi

@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.

TJ Holowaychuk
Owner

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

Merlyn

@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).

Merlyn

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

Nadav Ivgi

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

Merlyn

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

TJ Holowaychuk
Owner

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

Jonathan Ong
Collaborator

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
This page is out of date. Refresh to see the latest.

Showing 1 changed file with 3 additions and 0 deletions. Show diff stats Hide diff stats

  1. +3 0  bin/express
3  bin/express
@@ -348,6 +348,9 @@ function createApplicationAt(path) {
348 348 , scripts: { start: 'node app' }
349 349 , dependencies: {
350 350 express: version
  351 + }, engines: {
  352 + node: process.versions.node.replace(/\.\d+$/, '.x')
  353 + , npm: '1.1.x'
351 354 }
352 355 }
353 356

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.