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.

Sign up for GitHub

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

Jump to bottom

fix version inference regression #445

Merged
merged 1 commit into from
Aug 11, 2016
Merged

fix version inference regression #445

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
### Fixed

* [CLI] ensure --out has either a string or null value (#442)
* Use `get-package-info` (again) to support finding prebuilt in parent directories (#445)

## [7.5.1] - 2016-08-06

Expand Down
54 changes: 36 additions & 18 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ const debug = require('debug')('electron-packager')
const download = require('electron-download')
const extract = require('extract-zip')
const fs = require('fs-extra')
const prop = require('lodash.get')
const getPackageInfo = require('get-package-info')
const metadata = require('./package.json')
const os = require('os')
const path = require('path')
Expand Down Expand Up @@ -49,28 +49,46 @@ function validateList (list, supported, name) {
}

function getNameAndVersion (opts, dir, cb) {
var pkg = require(path.join(dir, 'package.json'))
var props = []
if (!opts.name) props.push(['productName', 'name'])
if (!opts.version) props.push(['dependencies.electron', 'devDependencies.electron'])

// Name and version provided, no need to infer
if (opts.name && opts.version) return cb(null)
if (props.length === 0) return cb(null)

// Infer name from package.json
opts.name = opts.name || prop(pkg, 'productName') || prop(pkg, 'name')

// Infer electron version from package.json
var electronVersion = prop(pkg, 'dependencies.electron') || prop(pkg, 'devDependencies.electron')
var electronPrebuiltVersion = prop(pkg, 'dependencies.electron-prebuilt') || prop(pkg, 'devDependencies.electron-prebuilt')
opts.version = opts.version || electronVersion || electronPrebuiltVersion

if (!electronVersion && !electronPrebuiltVersion) return cb(null)
// Search package.json files to infer name and version from
getPackageInfo(props, dir, function (err, result) {
if (err) {
// `get-package-info` exploded looking for `electron`. Try `electron-prebuilt` instead
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it also error if both productName and name were missing?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is electron different from electron-prebuilt? If they're the same, you can just do props.push(['dependencies.electron', 'dependencies.electron-prebuilt', 'devDependencies.electron', 'devDependencies.electron-prebuilt']), which will resolve to whichever it finds first.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

electron is preferred over electron-prebuilt (the former is the eventual replacement for the latter).

Copy link
Contributor

@rahatarmanahmed rahatarmanahmed Aug 8, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then it would be props.push(['dependencies.electron', 'devDependencies.electron', 'dependencies.electron-prebuilt', 'devDependencies.electron-prebuilt']). Either way, you can avoid a 2nd getPackageInfo call. I think that's all that was needed to add support for infering from electron.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushing all of them into the same props array was what I started with, but then we don't know if the resulting version was derived from electron or electron-prebuilt. Hence the separation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to know which package it came from?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because resolve needs a path:

resolve(packageName, {
  basedir: path.dirname(result.source[`dependencies.${packageName}`].src)
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, d'oh. I forgot about that. In that case it might be better to make a trivial PR to get-package-info that can add the property name to result.source here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went ahead and did that myself. Working on a PR for this branch to clean it up accordingly.

props.pop()
props.push(['dependencies.electron-prebuilt', 'devDependencies.electron-prebuilt'])
getPackageInfo(props, dir, function (err, result) {
if (err) return cb(err)
return inferNameAndVersionFromInstalled('electron-prebuilt', opts, result, cb)
})
} else {
return inferNameAndVersionFromInstalled('electron', opts, result, cb)
}
})
}

var packageName = electronVersion ? 'electron' : 'electron-prebuilt'
resolve(packageName, {basedir: dir}, function (err, res, pkg) {
if (err) return cb(err)
debug('Inferring target Electron version from `' + packageName + '` dependency or devDependency in package.json')
opts.version = pkg.version
function inferNameAndVersionFromInstalled (packageName, opts, result, cb) {
if (result.values.productName) {
debug('Inferring application name from productName or name in package.json')
opts.name = result.values.productName
}
if (result.values[`dependencies.${packageName}`]) {
resolve(packageName, {
basedir: path.dirname(result.source[`dependencies.${packageName}`].src)
}, function (err, res, pkg) {
if (err) return cb(err)
debug(`Inferring target Electron version from ${packageName} dependency or devDependency in package.json`)
opts.version = pkg.version
return cb(null)
})
} else {
return cb(null)
})
}
}

function createSeries (opts, archs, platforms) {
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
"electron-osx-sign": "^0.3.0",
"extract-zip": "^1.0.3",
"fs-extra": "^0.30.0",
"lodash.get": "^4.4.1",
"get-package-info": "^0.1.0",
"minimist": "^1.1.1",
"plist": "^1.1.0",
"rcedit": "^0.5.1",
Expand Down