Update get-package-info #505

rahatarmanahmed · 2016-10-13T05:10:42Z

Have you read the section in CONTRIBUTING.md about pull requests? Yes

Summarize your changes:

As discussed in #461, this updates get-package-info to v1.0.0, which includes more helpful errors when failing to find all info requested. This lets us give better error messages, as well as ignore missing props for optional options.

Are your changes appropriately documented? N/A

Do your changes have sufficient test coverage? Not sure yet.

Does the testsuite pass successfully on your local machine? Yes

This updates `get-package-info` to v1.0.0, which includes more helpful errors when failing to find all info requested. This lets us give better error messages, as well as ignore missing props for optional options.

rahatarmanahmed · 2016-10-13T05:11:43Z

index.js

+          }
+        })
+
+        err.message = messages.join('\n') + '\n' + err.message


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

original error message

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

rahatarmanahmed · 2016-10-13T05:12:19Z

index.js

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


Assuming app-version is not a required option.

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.

malept

In general this looks good. However, there are some style nits (we use Node 4+ compatible ES6) and some new code needs to be covered by tests.

malept · 2016-10-13T11:42:16Z

index.js

+      if (missingProps.filter(prop => requiredProps.find(reqProp => prop === reqProp)).length !== 0) {
+        var messages = missingProps.map(function (missingProp) {
+          missingProp = Array.isArray(missingProp) ? missingProp[0] : missingProp
+          if (missingProp === 'productName') {


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

malept · 2016-10-13T11:42:55Z

index.js

+          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'


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

malept · 2016-10-13T11:46:25Z

index.js

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


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.

malept · 2016-10-13T11:51:14Z

index.js

+          }
+        })
+
+        err.message = messages.join('\n') + '\n' + err.message


original error message

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

malept · 2016-10-13T11:52:32Z

index.js

+        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`)


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

malept · 2016-10-13T11:53:11Z

index.js

-    opts.name = result.values.productName
-  }
+    if (result.values.productName) {
+      debug('Inferring application name from productName or name in package.json')


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

malept · 2016-10-13T11:58:09Z

index.js

+  getPackageInfo(props, dir, (err, result) => {
+    if (err && err.missingProps) {
+      var requiredProps = ['productName', 'dependencies.electron']
+      var missingProps = err.missingProps.map(prop => {


malept · 2016-10-13T11:58:17Z

index.js

+
+      // Callback w/ error if there are required props that are missing
+      if (missingProps.filter(prop => requiredProps.find(reqProp => prop === reqProp)).length !== 0) {
+        var messages = missingProps.map(function (missingProp) {


Use let + fat arrow syntax here?

malept · 2016-10-13T11:58:23Z

index.js

+      // Callback w/ error if there are required props that are missing
+      if (missingProps.filter(prop => requiredProps.find(reqProp => prop === reqProp)).length !== 0) {
+        var messages = missingProps.map(function (missingProp) {
+          missingProp = Array.isArray(missingProp) ? missingProp[0] : missingProp


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

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

malept · 2016-10-13T12:00:31Z

index.js

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


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

rahatarmanahmed · 2016-10-13T15:04:56Z

Made changes from code review input. Looked into the test coverage, and it looks like the issue is stemming from the reason why @zeke opened #461 in the first place: it's picking up values from electron-packager's own package.json.

@malept's suggestions:

Copy a fixture to a temporary directory (so that the electron-packager package.json doesn't get detected by get-package-info) - and even then, there could be inconsistent failures on dev machines where someone puts a package.json in their root temporary directory

Refactor that code to stop at the first package.json - I'm generally in favor of this method, but the concern was that it would break existing code (even though that wasn't an explicit feature)

The issue with option 2 is that the feature of looking at parent package.json's is a required feature for project structures where app code is in a subdirectory with its own package.json. The idea is that the app package.json would have the productName, and the parent package.json would have a devDependency on electron. Not sure what best practice is nowadays, but I imagine a good number of projects do it this way.

I'm not so sure the issue in option 1 of having a package.json above the temp directory is going to be a huge issue. That seems like it'd be an extremely rare occurrence. If we need to, we can explicitly check if there is a stray package.json and fail the test w/ an informative error msg.

malept · 2016-10-13T23:55:33Z

I'm OK with you implementing option 1 in the way you describe.

zeke · 2016-10-14T01:23:34Z

looking at parent package.json's is a required feature for project structures where app code is in a subdirectory with its own package.json.

☝️ great point.

I'm OK with you implementing option 1 in the way you describe.

👍

rahatarmanahmed · 2016-10-14T21:30:11Z

test/basic.js

+  let fixtureDir = path.join(__dirname, 'fixtures', fixtureSubdir)
+  waterfall([
+    cb => fs.emptyDir(tmpdir, cb),
+    (cb1, cb2) => fs.copy(fixtureDir, tmpdir, cb2 || cb1), // inconsistent cb arguments from fs.emptyDir


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.

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

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

rahatarmanahmed · 2016-10-15T04:01:02Z

love too have tests fail at random on CI

I've run the failing tests on loop on my laptop but I can't figure out what's causing those rare failures. Otherwise, the code is ready to be re-reviewed. Hopefully something pops out during review that tells us about the failures.

eljefedelrodeodeljefe · 2016-10-22T16:48:50Z

I believe landing this is necessary to fix #513

eljefedelrodeodeljefe · 2016-10-22T16:50:21Z

Also highly doubtful that this module should be used at all.

malept · 2016-10-22T16:57:27Z

Also highly doubtful that this module should be used at all.

Why is that? Do you have an alternative proposal?

eljefedelrodeodeljefe · 2016-10-22T17:38:51Z

So using this is the higher-level of it's upstream dependencies which are already high level of try-catch-edJSON.parse(fs.readFile('package.json')) or require('package.json'). Either use its various upstream deps or those of the npm guys or just the above. This would decrease surface area of bugs.

rahatarmanahmed · 2016-10-23T20:17:32Z

If you wanna move the logic from get-package-info into electron-packager, that's fine by me. I initially made it a separate package so it wouldn't muck up the code here, and could be unit tested as well as tested thru electron-packager's integration tests.

(Also I could have sworn the tests were failing before? I don't know how to avoid the coverage decrease, since it's the line that's triggered on some kind of I/O failure looking for package.jsons.

malept

@rahatarmanahmed sorry about the delay, I've been busy.

If you wanna move the logic from get-package-info into electron-packager, that's fine by me.

I have no intention of moving the logic into Electron Packager. If it's not specific to how Electron Packager works, it shouldn't go in there.

Also I could have sworn the tests were failing before?

I reran the test. That's probably a bad sign about how inferring works that it times out intermittently.

malept · 2016-10-23T20:26:05Z

index.js

-      getPackageInfo(props, dir, function (err, result) {
-        if (err) return cb(err)
-        return inferNameAndVersionFromInstalled('electron-prebuilt', opts, result, cb)
+  getPackageInfo(props, dir, (err, result) => {


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

malept · 2016-10-23T20:27:08Z

index.js

+        result = err.result
+      }
+    } else if (err) {
+      return cb(err)


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

malept · 2016-10-23T20:29:37Z

index.js

-      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]


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

malept · 2016-10-23T20:33:12Z

test/basic.js

+}
+
+function createInferMissingVersionTest (opts) {
+  return function (t) {


Should use fat arrow function syntax when possible.

rahatarmanahmed · 2016-10-27T02:40:54Z

@malept moved the electron package.json reading into its own function, lmk if you have any better ideas on how to break it up even more.

Good call on the invalid package.json. Added a test for that. Should cover that line.

Tests seem to be failing due to some eslint peerdependency business, tho... 😞

malept · 2016-10-27T02:44:31Z

Tests seem to be failing due to some eslint peerdependency business, tho... 😞

WTF. I wanted to add Node 7 to CI anyway, I guess I need to mess with other dependencies too.

malept · 2016-10-27T05:04:54Z

Can't upgrade CI to use Node 7 until next week (blocked on AppVeyor). However, I upgraded eslint-plugin-promise on master and CI all passed the npm install portion.

malept

Apart from the refactors, 👍 looks good.

malept · 2016-10-27T05:09:06Z

index.js

-      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) {


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

malept · 2016-10-27T05:10:41Z

index.js

+
+      // Callback w/ error if there are required props that are missing
+      if (missingProps.filter(prop => requiredProps.find(reqProp => prop === reqProp)).length !== 0) {
+        let messages = missingProps.map(missingProp => {


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

rahatarmanahmed · 2016-10-27T18:31:57Z

greeeeeeeeen 🤑

zeke · 2016-10-30T04:55:58Z

Thanks for persevering, @rahatarmanahmed 👊

Update get-package-info

be3385d

This updates `get-package-info` to v1.0.0, which includes more helpful errors when failing to find all info requested. This lets us give better error messages, as well as ignore missing props for optional options.

rahatarmanahmed commented Oct 13, 2016

View reviewed changes

malept requested changes Oct 13, 2016

View reviewed changes

Code review changes

e5f10ee

rahatarmanahmed added 3 commits October 13, 2016 21:41

Move infer failure tests to temp dir

8f9427e

Add infer missing version only test

d735ad2

Fix fs.emptyDir returning inconsistent arguments

72ab0e7

rahatarmanahmed commented Oct 14, 2016

View reviewed changes

Make sure no package.jsons exist above tmpdir

a6a4616

malept requested changes Oct 23, 2016

View reviewed changes

Code review changes

2ad8aae

malept reviewed Oct 27, 2016

View reviewed changes

rahatarmanahmed added 2 commits October 27, 2016 11:01

Break up getMetadata

7853847

Remove object destructuring

96bddad

malept approved these changes Oct 29, 2016

View reviewed changes

malept merged commit ce86dc4 into electron:master Oct 29, 2016

Update get-package-info #505

Update get-package-info #505

Conversation

rahatarmanahmed commented Oct 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malept left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rahatarmanahmed commented Oct 13, 2016 • edited

malept commented Oct 13, 2016

zeke commented Oct 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rahatarmanahmed commented Oct 15, 2016

eljefedelrodeodeljefe commented Oct 22, 2016

eljefedelrodeodeljefe commented Oct 22, 2016

malept commented Oct 22, 2016

eljefedelrodeodeljefe commented Oct 22, 2016

rahatarmanahmed commented Oct 23, 2016

malept left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rahatarmanahmed commented Oct 27, 2016 • edited

malept commented Oct 27, 2016

malept commented Oct 27, 2016

malept left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rahatarmanahmed commented Oct 27, 2016

zeke commented Oct 30, 2016

rahatarmanahmed commented Oct 13, 2016 •

edited

rahatarmanahmed commented Oct 27, 2016 •

edited