Electron builder fix node issue #267

AWolf81 · 2018-09-26T20:18:16Z

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?

AWolf81 · 2018-09-26T20:22:29Z

src/electron.js

@@ -82,6 +113,7 @@ function createWindow() {
      slashes: true,
    });
  mainWindow.loadURL(startUrl);
+  mainWindow.toggleDevTools();


I'll remove toggleDevTools in my next commit. I think that was just needed for debugging.

joshwcomeau · 2018-09-27T15:01:51Z

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 · 2018-09-27T20:03:11Z

@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 · 2018-09-27T21:03:33Z

@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.

joshwcomeau

Wooo excited to see this land!

We should merge my fix-mac-nvm branch into this one before merging, but once that's done, let's do it!

joshwcomeau

[Edit - duplicate. Github was being funky for me last night]

* Fix bundled Mac app * Only replace path when necessary * Add env vars to exec

Jtfinlay · 2018-10-22T20:38:32Z

Appreciate you commandeering! Changes look great :)

AWolf81 · 2018-10-22T20:54:45Z

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 · 2018-10-22T20:59:50Z

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% <ø> (ø)`	⬆️
src/services/create-project.service.js	`31.81% <0%> (ø)`	⬆️
src/components/CreateNewProjectWizard/BuildPane.js	`0% <0%> (ø)`	⬆️
src/electron.js	`0% <0%> (ø)`
src/components/Initialization/Initialization.js	`32.25% <0%> (-1.08%)`	⬇️
src/services/platform.service.js	`58.33% <45.45%> (-15.01%)`	⬇️

AWolf81 · 2018-10-23T07:03:48Z

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.

AWolf81 · 2018-10-23T10:34:42Z

package.json

@@ -75,7 +85,8 @@
    "dotenv": "5.0.0",
    "dotenv-expand": "4.2.0",
    "electron": "2.0.1",
-    "electron-packager": "12.1.0",
+    "electron-builder": "20.28.4",
+    "electron-log": "^2.2.17",


Nit: Version not pinned.
Feel free to remove the ^ but I think we could also fix this after merging.

Yeah, I'm not too worried.

joshwcomeau

Woooohoooooo!

joshwcomeau · 2018-10-23T10:38:12Z

package.json

@@ -75,7 +85,8 @@
    "dotenv": "5.0.0",
    "dotenv-expand": "4.2.0",
    "electron": "2.0.1",
-    "electron-packager": "12.1.0",
+    "electron-builder": "20.28.4",
+    "electron-log": "^2.2.17",


Yeah, I'm not too worried.

joshwcomeau · 2018-10-23T10:39:19Z

src/services/platform.service.js

+    [pathKey]:
+      currentEnvironment[pathKey] +
+      path.join(projectPath, 'node_modules', '.bin', path.delimiter),
+  };


Makes sense 👍

Jtfinlay and others added 25 commits September 9, 2018 00:57

Have electron-builder working with webpack

ddaed5f

Successfully able to spawn yarn

e444544

include icon

ab29ee2

Rename to electron, and clean up

82a20ac

512x512 icon for mac

23d5997

sync with master

3b44b51

Update electron asset ref

93e25ea

Have paths working correctly for assets needed from electronjs

2631c76

add linux to dist:all

66a3738

Include the precommit

ffd2a59

wip

afdee72

fix image paths

e40d55a

remove extra indent changes

f2ef82a

added NODE env. variable

3d182ae

added shell to spawn

225e34b

Merge branch 'master' into buildermin-fix-node

e8de216

WIP: Added electron-updater

1e66c54

fixed regeneratorRuntime issue

edafc1f

added shell

284ec8f

bumped version for testing

c300d50

modified publish & set to private (npm publish)

ce77b8e

new test version

a7fbb91

changed versioning - no update notification with prev. release

a4a4665

bumped version. Updater not triggered.

2015dd1

Removed debug logs. Updater setup OK.

70b9b5d

AWolf81 commented Sep 26, 2018

View reviewed changes

AWolf81 added 2 commits September 26, 2018 23:10

changed devtools setup

b30f09c

added shell & env

ed88fa9

AWolf81 requested review from joshwcomeau and superhawk610 September 26, 2018 22:09

archont00 mentioned this pull request Sep 30, 2018

Set StartupWMClass=Joplin in the appimage.desktop file laurent22/joplin#338

Closed

8 tasks

joshwcomeau mentioned this pull request Oct 4, 2018

Set up CI to handle releases and code-signing #272

Open

6 tasks

joshwcomeau approved these changes Oct 22, 2018

View reviewed changes

joshwcomeau and others added 3 commits October 22, 2018 19:42

Fix bundled Mac app (#291)

67b0a2e

* Fix bundled Mac app * Only replace path when necessary * Add env vars to exec

fixed conflicts

33e8ff6

added ignore for failing graphql type

952c8d1

Path fix for Windows

10fe280

Changed to release version

73b2969

AWolf81 commented Oct 23, 2018

View reviewed changes

joshwcomeau approved these changes Oct 23, 2018

View reviewed changes

joshwcomeau merged commit 8e0c83f into master Oct 23, 2018

joshwcomeau deleted the buildermin-fix-node branch October 23, 2018 10:40

AWolf81 mentioned this pull request Nov 1, 2018

WIP: #26 Move to electron-builder #236

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Electron builder fix node issue #267

Electron builder fix node issue #267

AWolf81 commented Sep 26, 2018

AWolf81 Sep 26, 2018

joshwcomeau commented Sep 27, 2018 •

edited

Loading

AWolf81 commented Sep 27, 2018

joshwcomeau commented Sep 27, 2018 •

edited

Loading

joshwcomeau left a comment

joshwcomeau left a comment •

edited

Loading

Jtfinlay commented Oct 22, 2018

AWolf81 commented Oct 22, 2018 •

edited

Loading

codecov bot commented Oct 22, 2018 •

edited

Loading

AWolf81 commented Oct 23, 2018

AWolf81 Oct 23, 2018

joshwcomeau Oct 23, 2018

joshwcomeau left a comment

joshwcomeau Oct 23, 2018

joshwcomeau Oct 23, 2018

Electron builder fix node issue #267

Electron builder fix node issue #267

Conversation

AWolf81 commented Sep 26, 2018

AWolf81 Sep 26, 2018

Choose a reason for hiding this comment

joshwcomeau commented Sep 27, 2018 • edited Loading

AWolf81 commented Sep 27, 2018

joshwcomeau commented Sep 27, 2018 • edited Loading

joshwcomeau left a comment

Choose a reason for hiding this comment

joshwcomeau left a comment • edited Loading

Choose a reason for hiding this comment

Jtfinlay commented Oct 22, 2018

AWolf81 commented Oct 22, 2018 • edited Loading

codecov bot commented Oct 22, 2018 • edited Loading

Codecov Report

AWolf81 commented Oct 23, 2018

AWolf81 Oct 23, 2018

Choose a reason for hiding this comment

joshwcomeau Oct 23, 2018

Choose a reason for hiding this comment

joshwcomeau left a comment

Choose a reason for hiding this comment

joshwcomeau Oct 23, 2018

Choose a reason for hiding this comment

joshwcomeau Oct 23, 2018

Choose a reason for hiding this comment

joshwcomeau commented Sep 27, 2018 •

edited

Loading

joshwcomeau commented Sep 27, 2018 •

edited

Loading

joshwcomeau left a comment •

edited

Loading

AWolf81 commented Oct 22, 2018 •

edited

Loading

codecov bot commented Oct 22, 2018 •

edited

Loading