fix version inference regression

[zeke]

Fixes #441

This brings get-package-info back into the mix, so directories are properly traversed for package.json files containing electron or electron-prebuilt. The work is broken up into two functions now to work around the resolve issue that causes an error to be thrown when a given set of props are not found.

cc @malept @kevinsawicki

[malept]

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

[rahatarmanahmed]

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.

[malept]

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

[rahatarmanahmed]

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.

[zeke]

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.

[rahatarmanahmed]

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

[zeke]

Because resolve needs a path:

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

[rahatarmanahmed]

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

[rahatarmanahmed]

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

[zeke]

Now using ES6 string interpolation. I was avoiding it because I thought maybe this project needed to work on older versions of node. But no!

  "engines": {
    "node": ">= 4.0"
  }

[malept]

I was avoiding it because I thought maybe this project needed to work on older versions of node. But no!

Yep. And I'm paying for that decision because you can't actually enforce a Node engine version as far as I know. 😢

[zeke]

Thanks for the patch, @rahatarmanahmed. I merged #446. If this passes CI, let's ship it!

[malept]

Sounds good. Just needs a NEWS entry and some rebasing.

[malept]

There's that error again 😞

[zeke]

Added news, squashed. Last CI run passed on all six builds. This is ready. :fingers_crossed:

[malept]

Argh https://travis-ci.org/electron-userland/electron-packager/jobs/151351051

[zeke]

Rerunning that build. Pretty sure this is just more flakiness.

[malept]

Doesn't really fix #441 then, right?

[MarshallOfSound]

Yep. And I'm paying for that decision because you can't actually enforce a Node engine version as far as I know.

@malept You can be really mean and do something like this in index.js

if (parseInt(process.versions.node[0], 10) < 4) {
  throw new BigNastyError('Get a better node version plz')
}

[malept]

@MarshallOfSound I would, but I think I get a SyntaxError before that can be executed.

[zeke]

Doesn't really fix #441 then, right?

The flakiness on Travis has not been unique to Node 4. I was seeing it on all node versions and platforms. I think this is ready to 🚢

[malept]

As far as I can tell, it's always been with that specific test, though. Which is really what #441 is about.

[zeke]

@malept what do you say we ship this since it is definitely an improvement to a known bug in the latest release? We can address the flakiness if it turns out to be an issue.

[malept]

Sure. The only thing I object to in this PR is the assertion that it fixes #441.