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 1 commit
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
103 changes: 61 additions & 42 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,50 +23,77 @@ 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) {
var requiredProps = ['productName', 'dependencies.electron']
var missingProps = err.missingProps.map(prop => {
Copy link
Member

Choose a reason for hiding this comment

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

let

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.

var messages = missingProps.map(function (missingProp) {
Copy link
Member

Choose a reason for hiding this comment

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

Use let + fat arrow syntax here?

missingProp = Array.isArray(missingProp) ? missingProp[0] : missingProp
Copy link
Member

Choose a reason for hiding this comment

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

Didn't you already do this earlier when declaring missingProps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whoops. i really shouldn't make PRs at midnight on a work day

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.

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

if (missingProp === 'dependencies.electron') {
return 'Unable to determine Electron version. Please specify an Electron version\n\n' +
'For more information, please see\n' +
'https://github.com/electron-userland/electron-packager/blob/master/docs/api.md#version\n'
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice if these error messages could be DRY'd up somehow.

}
})

err.message = messages.join('\n') + '\n' + err.message
Copy link
Contributor Author

Choose a reason for hiding this comment

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

May want to hide original error message, since the message we add should be descriptive enough without it?

Copy link
Member

Choose a reason for hiding this comment

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

original error message

The one that's sent by get-package-info? If so, perhaps it should be printed in a debug() message.

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 productName or name in package.json')
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this debug statement more specific now with result.source?

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 package.json')
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this debug statement more specific now with result.source?

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`]) {
var packageName = result.source[`dependencies.electron`].prop.split('.')[1]
Copy link
Member

Choose a reason for hiding this comment

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

let (I need to add ES6 rules to eslint...)

resolve(packageName, {
basedir: path.dirname(result.source[`dependencies.electron`].src)
}, function (err, res, pkg) {
if (err) return cb(err)
debug(`Inferring target Electron version from ${packageName} dependency or devDependency in package.json`)
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this debug statement more specific now with result.source?

opts.version = pkg.version
return cb(null)
})
} else {
return cb(null)
})
} else {
return cb(null)
}
}
})
}

function createSeries (opts, archs, platforms) {
Expand Down Expand Up @@ -198,15 +225,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
2 changes: 1 addition & 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 Down