Electron builder fix node issue

[AWolf81]

Related Issue:

26

Hey @Jtfinlay thanks for your work on this. I hope it's OK that I'm commandeering this as you're busy with wedding planning. Hope everything is going well with your planning. It would be also great if you could have a look - just if you're not too busy.

Summary:

• This branch is based on buildermin from jtfinlay's fork.
• Added shell: true to spawn for launchDevServer & taskRun. I'll add it to createNewProject in a minute. I think that's open.
• Verified that eject is still working. No issue detected on Windows.
• Checked electron auto updater setup & it is working. UX is OK but could be improved later. At the moment it will download the new version in the background and displays a dialog message so the user can decide restart now or update later. Requirements so updating is working:
  • Pre-releases are not detected by updater - so we need to do official releases.
  • latest.yml required in release but we're having this automatically if yarn run publish is used. (Attention: Don't use yarn publish, omitted run, as this is trying to publish to npmjs. Maybe we could rename the publish command to avoid the confusion.)
  • *.exe.blockmap used for differential download - so keep it in the release. If it's not present it will fallback to full download.
  • Updater is using semver isGreaterThan method. So it's important to have the semversion right and I would avoid alpha releases as this is a bit confusing. So if we'd like to deploy a version than it should be a release like 0.3.x.

@joshwcomeau & @superhawk610 can you please test on Mac and Ubuntu?

[joshwcomeau]

Hey @AWolf81,

Thanks so much for your work on this! Afraid I don't have time to give this a thorough review right now, but I gave it a quick test run. It runs great in development, but I'm seeing this issue again, after installing the application:

I believe I've seen this issue on an unrelated branch, as well, so I'm not convinced it's a problem with this change. I'll dig some more this evening, if I have the time.

[AWolf81]

@joshwcomeau I've seen this issue before but I'm not sure how I fixed it - maybe refresh state helped. I couldn't re-create the issue that's why I haven't reported it. So as you mentioned this is unrelated but it would be great to find the cause - looks a bit like the package.json wasn't created on disk or maybe a permission issue.

One issue that's in the branch is that the app icon is not correctly displayed on Linux. It displays the following icon on Ubuntu 16.04:

Not sure how to fix it. Seems like others also have that problem - see issue. Just the icon is not correctly displayed - other functionality is working (tested devServer, build, test & eject) on Ubuntu.

[joshwcomeau]

@joshwcomeau I've seen this issue before but I'm not sure how I fixed it - maybe refresh state helped

Yeah, so this is reproducible for me, and it happens with a fresh state (I didn't actually try resetting, but there was no state to refresh).

I'll spend some time soon checking to see if it happens on master. Because yeah, I'm not convinced this change is to blame, but if it's broken on master, this is release-blocking, so we should fix it ASAP anyway.

But yeah I can take the lead on that since it's a MacOS-exclusive issue.

One issue that's in the branch is that the app icon is not correctly displayed on Linux. It displays the following icon on Ubuntu 16.04

Ah, hm. That's a bit of a shame. The linked issue has a solution about adding an icon prop to BrowserWindow, but we are doing that already, so maybe there are new restrictions to the filesize or file format or something... although that shouldn't change from the builder, that's a core Electron API.

I think that should become its own issue, after this lands, but I don't think it should be blocking.

Also: we should update our analytics to track platform. If only 1% of our users wind up being linux users (which wouldn't surprise me), I'm not so concerned about cosmetic defects that don't affect functionality. I'll create an issue for that.

[Jtfinlay]

Appreciate you commandeering! Changes look great :)

[AWolf81]

You're welcome @Jtfinlay.

OK, I've prepared everything but I think we need to check one final point before merging to master.

I'm getting again that react-scripts were not found error like mentioned here during development & in the binary. But this time I can not get it to work with-out a change.

Ubuntu not tested yet.

There was a flow error inside node_modules for graphql. Not sure why that happend but I added it to flow ignore as this is unrelated to Guppy.

[codecov[bot]]

Codecov Report

Merging #267 into master will decrease coverage by 0.02%. The diff coverage is 20.83%.

@@            Coverage Diff             @@
##           master     #267      +/-   ##
==========================================
- Coverage   20.52%   20.49%   -0.03%     
==========================================
  Files         239      239              
  Lines        3713     3733      +20     
  Branches      381      385       +4     
==========================================
+ Hits          762      765       +3     
- Misses       2676     2690      +14     
- Partials      275      278       +3

Impacted Files	Coverage Δ
src/sagas/task.saga.js	60.68% <ø> (ø)	:arrow_up:
src/services/create-project.service.js	31.81% <0%> (ø)	:arrow_up:
src/components/CreateNewProjectWizard/BuildPane.js	0% <0%> (ø)	:arrow_up:
src/electron.js	0% <0%> (ø)
src/components/Initialization/Initialization.js	32.25% <0%> (-1.08%)	:arrow_down:
src/services/platform.service.js	58.33% <45.45%> (-15.01%)	:arrow_down:

[AWolf81]

Yay🎉! I've found how to fix the issue on Windows.

It was because the path key on Windows started with an upper-case P and so adding to PATH caused two path keys. Then exec/spawn probably took the PATH value and was missing the defaults from Path.

I think this is ready for merging - I just haven't re-tested it on Ubuntu.