Add support for the new `electron` package name #435

zeke · 2016-08-02T18:38:34Z

A rename of electron-prebuilt to electron is in the works: electron-userland/electron-prebuilt#160

Bothelectron-prebuilt and electron will be published in tandem through the end of 2016.

This PR updates electron-packager to add support for users of the electron package, while maintaining existing functionality for electron-prebuilt.

@kevinsawicki @jlord @zcbenz @iolsen

malept · 2016-08-02T20:58:11Z

You could probably minimize the new electron fixture.

zeke · 2016-08-03T06:50:45Z

I have a fix in place and the test suite is passing for me locally but CI is failing.

I removed get-package-info because I was hitting an issue described right in its readme:

This is an error from resolve that means it was not able to find all the requested props. It would be nice to have a more descriptive error but there's no way to do that until this PR is merged.

I changed that code to just require the package.json file in the given directory, then use lodash.get to go spelunking for deep properties.

I will dig into the CI failures tomorrow.

malept · 2016-08-03T14:36:47Z

NEWS.md

@@ -2,6 +2,12 @@

 ## Unreleased

+## [7.5.0] - 2016-08-03


If you're adding this, I assume you're going to handle releasing a new version as soon as you merge. Otherwise, I suggest leaving it off and then the release will happen when it happens (mostly so the date is accurate).

Also, add a link comparing 7.4.0 to 7.5.0 like the other versions (including 7.4.0 which I just fixed in c4809e1)

zeke · 2016-08-03T19:39:17Z

Okay this is ready for review.

The previous push passed on 5 of 6 CI boxes; hopefully we'll see all green this time. (CI takes about an hour)

zeke · 2016-08-03T19:39:54Z

Happy to squash commits if that is still the convention here.

malept · 2016-08-03T20:43:09Z

Happy to squash commits if that is still the convention here.

Yup, see https://github.com/electron-userland/electron-packager/blob/master/CONTRIBUTING.md#filing-pull-requests (last bullet point)

I will try to review later tonight. (Though I just noticed that somehow test coverage decreased with this PR...hopefully it's because CI isn't done yet.)

malept · 2016-08-04T15:25:33Z

CONTRIBUTING.md

+TRAVIS_OS_NAME=osx ./test/ci/before_install.sh
+```
+
+If you're using Linux:


If you're using a Debian/Ubuntu-derived distribution of Linux:

Also for Linux, that script is specifically for x86_64 architectures (since Travis only provides those machines). Not sure if it will error if you try to run it on an x86 architecture (mostly because I haven't tried it).

malept · 2016-08-04T15:40:36Z

Hmmm. The tests no longer hit the "unable to find app name/electron version" code? Also, the test failure on Linux/Node 6 is troubling, but I'm not sure why it's happening (I reran the test run on Travis with the same result).

zeke · 2016-08-04T19:21:04Z

Thanks for all the feedback, @malept. I pushed up some changes to the docs and DRY'd things a bit.

I feel like the npm install stuff can live without being optimized for now.

As for the CI, the tests definitely seem flaky. I keep seeing 5/6 passed builds, but the failing environment is not consistently the same. Let's see what happens on this go around.

malept · 2016-08-04T19:38:00Z

NEWS.md

+
+### Added
+
+* Support the new `electron` package name #435


Parentheses around the issue number, please (for consistency)

malept · 2016-08-04T19:49:20Z

I have not been thrilled with this project's test runner (or tests, for that matter). Any suggestions/improvements would be greatly appreciated.

zeke · 2016-08-04T19:59:16Z

Travis results from three recent pushes (without meaningful code changes):

zeke · 2016-08-04T20:01:59Z

Squashed it all down into a single commit. Pretty sure this is ready to go. Let's defer on improving the test suite for another PR.

malept · 2016-08-04T20:05:02Z

I'm 👍 except for that one new infer test, which I'd like to see fixed before merging.

Let's defer on improving the test suite for another PR.

Agreed.

zeke · 2016-08-04T20:06:15Z

except for that one new infer test

What's wrong with it?

malept · 2016-08-04T20:09:03Z

It's mostly duplicate code from the other test (pretty much everything except 3 lines).

zeke · 2016-08-04T20:28:10Z

Simplified the test in 61ba66c (good call), and re-squashed commits.

malept · 2016-08-04T23:10:34Z

@zeke Thanks! Please release 😄

zeke · 2016-08-04T23:49:29Z

Thanks for helping out, @malept. Landed in 7.5.0

SimulatedGREG · 2016-08-06T17:49:52Z

Hmmm. The tests no longer hit the "unable to find app name/electron version" code? Also, the test failure on Linux/Node 6 is troubling, but I'm not sure why it's happening (I reran the test run on Travis with the same result).

The 7.5.0 update seems to have broken my setup as I keep getting [Error: must specify version] when building. I then tried installing [email protected] and used same configuration with no problems.
The item that seems to fix this issue would be having to define the version of electron manually that I'd like to package with. Yet as of the docs I shouldn't have to.

If omitted, it will use the version of the nearest local installation of electron or electron-prebuilt, defined in package.json in either dependencies or devDependencies.

Can confirm this happens with both node 5.10.0 and 6.2.0 on macOS. Here is my full error message...

Error: must specify version
    at download (/Users/gregholguin/Desktop/testing123/node_modules/electron-download/index.js:16:27)
    at Array.<anonymous> (/Users/gregholguin/Desktop/testing123/node_modules/electron-packager/index.js:121:7)
    at each (/Users/gregholguin/Desktop/testing123/node_modules/run-series/index.js:17:24)
    at next (/Users/gregholguin/Desktop/testing123/node_modules/rimraf/rimraf.js:74:7)
    at FSReqWrap.CB [as oncomplete] (/Users/gregholguin/Desktop/testing123/node_modules/rimraf/rimraf.js:110:9)

UPDATE: Currently using electron-prebuilt.

malept · 2016-08-06T18:32:06Z

@SimulatedGREG I'm in the middle of releasing a fix (see #439 and #440).

SimulatedGREG · 2016-08-06T18:39:36Z

@malept was just noticing. Thanks for the awesome work! Scary how switching from get-package-info to lodash.get created such a problem 😁.

malept · 2016-08-06T18:41:22Z

It basically means we need help with a better testsuite.

zeke force-pushed the support-renamed-prebuilt branch from 41be860 to cefb2b8 Compare August 3, 2016 06:35

malept reviewed Aug 3, 2016
View reviewed changes

zeke changed the title ~~[WIP] Add support for the new electron package name~~ Add support for the new electron package name Aug 3, 2016

malept reviewed Aug 4, 2016
View reviewed changes

NEWS.md

### Added

* Support the new `electron` package name #435

Copy link

Member

malept Aug 4, 2016

Choose a reason for hiding this comment

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

Parentheses around the issue number, please (for consistency)

zeke force-pushed the support-renamed-prebuilt branch from 6b63678 to 8aa7dfc Compare August 4, 2016 20:01

support the new electron package name

c508500

zeke force-pushed the support-renamed-prebuilt branch from 61ba66c to c508500 Compare August 4, 2016 20:27

malept merged commit 9cae421 into master Aug 4, 2016

malept deleted the support-renamed-prebuilt branch August 4, 2016 23:10

malept mentioned this pull request Aug 5, 2016

Fix infer electron test introduced in 7.5.0 #441

Closed

SimulatedGREG mentioned this pull request Aug 6, 2016

Error: must specify version SimulatedGREG/electron-vue#12

Closed

malept mentioned this pull request Aug 6, 2016

Fix dir of getNameAndVersion for require package.json #440

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for the new `electron` package name #435

Add support for the new `electron` package name #435

zeke commented Aug 2, 2016 •

edited

Loading

malept commented Aug 2, 2016

zeke commented Aug 3, 2016

malept Aug 3, 2016

malept Aug 4, 2016

zeke commented Aug 3, 2016

zeke commented Aug 3, 2016

malept commented Aug 3, 2016

malept Aug 4, 2016

malept Aug 4, 2016

malept commented Aug 4, 2016

zeke commented Aug 4, 2016

malept Aug 4, 2016

malept commented Aug 4, 2016

zeke commented Aug 4, 2016

zeke commented Aug 4, 2016

malept commented Aug 4, 2016

zeke commented Aug 4, 2016

malept commented Aug 4, 2016

zeke commented Aug 4, 2016

malept commented Aug 4, 2016

zeke commented Aug 4, 2016

SimulatedGREG commented Aug 6, 2016 •

edited

Loading

malept commented Aug 6, 2016

SimulatedGREG commented Aug 6, 2016

malept commented Aug 6, 2016

Add support for the new electron package name #435

Add support for the new electron package name #435

Conversation

zeke commented Aug 2, 2016 • edited Loading

malept commented Aug 2, 2016

zeke commented Aug 3, 2016

malept Aug 3, 2016

Choose a reason for hiding this comment

malept Aug 4, 2016

Choose a reason for hiding this comment

zeke commented Aug 3, 2016

zeke commented Aug 3, 2016

malept commented Aug 3, 2016

malept Aug 4, 2016

Choose a reason for hiding this comment

malept Aug 4, 2016

Choose a reason for hiding this comment

malept commented Aug 4, 2016

zeke commented Aug 4, 2016

malept Aug 4, 2016

Choose a reason for hiding this comment

malept commented Aug 4, 2016

zeke commented Aug 4, 2016

zeke commented Aug 4, 2016

malept commented Aug 4, 2016

zeke commented Aug 4, 2016

malept commented Aug 4, 2016

zeke commented Aug 4, 2016

malept commented Aug 4, 2016

zeke commented Aug 4, 2016

SimulatedGREG commented Aug 6, 2016 • edited Loading

malept commented Aug 6, 2016

SimulatedGREG commented Aug 6, 2016

malept commented Aug 6, 2016

Add support for the new `electron` package name #435

Add support for the new `electron` package name #435

zeke commented Aug 2, 2016 •

edited

Loading

SimulatedGREG commented Aug 6, 2016 •

edited

Loading