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 get-package-info #505

Merged
merged 9 commits into from
Oct 29, 2016
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
108 changes: 67 additions & 41 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,50 +23,84 @@ function getMetadata (opts, dir, cb) {
var props = []
if (!opts.name) props.push(['productName', 'name'])
if (!opts['app-version']) props.push('version')
if (!opts.version) props.push(['dependencies.electron', 'devDependencies.electron'])
if (!opts.version) {
props.push([
'dependencies.electron',
'devDependencies.electron',
'dependencies.electron-prebuilt',
'devDependencies.electron-prebuilt'
])
}

// Name and version provided, no need to infer
if (props.length === 0) 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
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)
getPackageInfo(props, dir, (err, result) => {
Copy link
Member

Choose a reason for hiding this comment

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

This function has gotten rather large. Can it be split up?

if (err && err.missingProps) {
let requiredProps = ['productName', 'dependencies.electron']
let missingProps = err.missingProps.map(prop => {
return Array.isArray(prop) ? prop[0] : prop
})
} else {
return inferNameAndVersionFromInstalled('electron', opts, result, cb)

// Callback w/ error if there are required props that are missing
if (missingProps.filter(prop => requiredProps.find(reqProp => prop === reqProp)).length !== 0) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd extract the conditional into a method for readability (and also lets you get rid of the comment.

let messages = missingProps.map(missingProp => {
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps extract the map function into a top-level function named errorMessageForProperty?

let hash, propName
if (missingProp === 'productName') {
Copy link
Member

Choose a reason for hiding this comment

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

Looks like you need to add a test for this conditional.

hash = 'name'
propName = 'application name'
}

if (missingProp === 'dependencies.electron') {
hash = 'version'
propName = 'Electron version'
}

return `Unable to determine ${propName}. Please specify an ${propName}\n\n` +
'For more information, please see\n' +
`https://github.com/electron-userland/electron-packager/blob/master/docs/api.md#${hash}\n`
})

debug(err.message)
err.message = messages.join('\n') + '\n'
return cb(err)
} else {
// Missing props not required, can continue w/ partial result
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Assuming app-version is not a required option.

Copy link
Member

Choose a reason for hiding this comment

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

app-version isn't required. From docs/api.md:

If neither [app-version option nor version from package.json] are provided, the version of Electron will be used.

result = err.result
}
} else if (err) {
return cb(err)
Copy link
Member

Choose a reason for hiding this comment

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

Would an IO error include an invalid package.json? Just trying to think of ways to test this line.

}
})
}

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.productName) {
debug(`Inferring application name from ${result.source.productName.prop} in ${result.source.productName.src}`)
opts.name = result.values.productName
}

if (result.values.version) {
debug('Inferring app-version from version in package.json')
opts['app-version'] = result.values.version
}
if (result.values.version) {
debug(`Inferring app-version from version in ${result.source.version.src}`)
opts['app-version'] = result.values.version
}

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
if (result.values['dependencies.electron']) {
let {prop, src} = result.source['dependencies.electron']
return getVersion(opts, prop.split('.')[1], src, cb)
} else {
return cb(null)
})
} else {
}
})
}

function getVersion (opts, packageName, src, cb) {
resolve(packageName, {
basedir: path.dirname(src)
}, (err, res, pkg) => {
if (err) return cb(err)
debug(`Inferring target Electron version from ${packageName} in ${src}`)
opts.version = pkg.version
return cb(null)
}
})
}

function createSeries (opts, archs, platforms) {
Expand Down Expand Up @@ -198,15 +232,7 @@ module.exports = function packager (opts, cb) {
debug(`Target Architectures: ${archs.join(', ')}`)

getMetadata(opts, path.resolve(process.cwd(), opts.dir) || process.cwd(), function (err) {
if (err) {
err.message = 'Unable to determine application name or Electron version. ' +
'Please specify an application name and Electron version.\n\n' +
'For more infomation, please see \n' +
'https://github.com/electron-userland/electron-packager/blob/master/docs/api.md#name or \n' +
'https://github.com/electron-userland/electron-packager/blob/master/docs/api.md#version\n\n' +
err.message
return cb(err)
}
if (err) return cb(err)

debug(`Application name: ${opts.name}`)
debug(`Target Electron version: ${opts.version}`)
Expand Down
3 changes: 2 additions & 1 deletion package.json
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",
"get-package-info": "^0.1.0",
"get-package-info": "^1.0.0",
"minimist": "^1.1.1",
"plist": "^2.0.0",
"rcedit": "^0.7.0",
Expand All @@ -41,6 +41,7 @@
"eslint-plugin-standard": "^2.0.0",
"is-admin": "^2.0.0",
"nyc": "^8.0.0",
"pkg-up": "^1.0.0",
"rcinfo": "^0.1.3",
"rimraf": "^2.3.2",
"run-waterfall": "^1.1.1",
Expand Down
59 changes: 54 additions & 5 deletions test/basic.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,13 @@
const common = require('../common')
const config = require('./config.json')
const fs = require('fs-extra')
const os = require('os')
const packager = require('..')
const path = require('path')
const test = require('tape')
const util = require('./util')
const waterfall = require('run-waterfall')
const pkgUp = require('pkg-up')

// Generates a path to the generated app that reflects the name given in the options.
// Returns the Helper.app location on darwin since the top-level .app is already tested for the
Expand Down Expand Up @@ -265,16 +267,57 @@ function createInferElectronTest (opts) {
}
}

function copyFixtureToTempDir (fixtureSubdir, cb) {
let tmpdir = path.join(os.tmpdir(), fixtureSubdir)
let fixtureDir = path.join(__dirname, 'fixtures', fixtureSubdir)
waterfall([
cb => {
let tmpdirPkg = pkgUp.sync(path.join(tmpdir, '..'))
if (tmpdirPkg) return cb(new Error(`Found package.json in parent of temp directory, which will interfere with test results. Please remove package.json at ${tmpdirPkg}`))
cb()
},
cb => fs.emptyDir(tmpdir, cb),
(cb1, cb2) => fs.copy(fixtureDir, tmpdir, cb2 || cb1), // inconsistent cb arguments from fs.emptyDir
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is weird b/c fs.emptyDir doesn't return a consistent # of arguments, depending on whether the directory already exists or not. I've made an issue on the fs-extra repo but this is the workaround for now.

Copy link
Member

Choose a reason for hiding this comment

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

I've made an issue on the fs-extra repo

For reference: jprichardson/node-fs-extra#287

cb => cb(null, tmpdir)
], cb)
}

function createInferFailureTest (opts, fixtureSubdir) {
return function (t) {
t.timeoutAfter(config.timeout)

delete opts.version
opts.dir = path.join(__dirname, 'fixtures', fixtureSubdir)
copyFixtureToTempDir(fixtureSubdir, (err, dir) => {
if (err) return t.end(err)

packager(opts, function (err, paths) {
t.ok(err, 'error thrown')
t.end()
delete opts.version
opts.dir = dir

packager(opts, function (err, paths) {
t.ok(err, 'error thrown')
t.end()
})
})
}
}

function createInferMissingVersionTest (opts) {
return (t) => {
t.timeoutAfter(config.timeout)
copyFixtureToTempDir('infer-missing-version-only', (err, dir) => {
if (err) return t.end(err)

delete opts.version
opts.dir = dir
let packageJSON = require(path.join(opts.dir, 'package.json'))

packager(opts, (err, paths) => {
if (!err) {
var version = fs.readFileSync(path.join(paths[0], 'version'), 'utf8')
t.equal(`v${packageJSON.devDependencies['electron']}`, version.toString(), 'The version should be inferred from installed `electron` version')
}

t.end(err)
})
})
}
}
Expand All @@ -287,6 +330,10 @@ function createInferWithBadFieldsTest (opts) {
return createInferFailureTest(opts, 'infer-bad-fields')
}

function createInferWithMalformedJSONTest (opts) {
return createInferFailureTest(opts, 'infer-malformed-json')
}

function createTmpdirTest (opts) {
return function (t) {
t.timeoutAfter(config.timeout)
Expand Down Expand Up @@ -406,6 +453,8 @@ util.testSinglePlatform('infer test using `electron-prebuilt` package', createIn
util.testSinglePlatform('infer test using `electron` package', createInferElectronTest)
util.testSinglePlatform('infer missing fields test', createInferMissingFieldsTest)
util.testSinglePlatform('infer with bad fields test', createInferWithBadFieldsTest)
util.testSinglePlatform('infer with malformed JSON test', createInferWithMalformedJSONTest)
util.testSinglePlatform('infer with missing version only test', createInferMissingVersionTest)
util.testSinglePlatform('defaults test', createDefaultsTest)
util.testSinglePlatform('out test', createOutTest)
util.testSinglePlatform('prune test', (baseOpts) => {
Expand Down
2 changes: 2 additions & 0 deletions test/fixtures/infer-malformed-json/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
{
"productName": "InferMalformedJSON",
7 changes: 7 additions & 0 deletions test/fixtures/infer-missing-version-only/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"main": "main.js",
"productName": "MainJS",
"devDependencies": {
"electron": "1.3.1"
}
}
1 change: 1 addition & 0 deletions test/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ function npmInstallforFixtures () {
const fixtures = [
'basic',
'basic-renamed-to-electron',
'infer-missing-version-only',
'el-0374'
]
return fixtures.map((fixture) => {
Expand Down