Skip to content
This repository has been archived by the owner on Jun 17, 2021. It is now read-only.

WIP: #26 Move to electron-builder #236

Closed
wants to merge 13 commits into from

Conversation

Jtfinlay
Copy link

@Jtfinlay Jtfinlay commented Sep 9, 2018

Related Issue: #26

Summary:

Moving from electron-packager to electron-builder.

electron-builder is pulling the files needed from '/build'. The electron starter script (main.js, now renamed to electron.js to prevent confusion) has shared dependencies with the main app, like references to 'kill-process-id.service' and 'electron-store.service'. To solve this, made an 'electron' entry for webpack. This way, it pulls whatever dependencies are needed, keeps build/electron.js and build/main.js separate, but allows code to be shared between them in the source.

Set yarn node_modules dependency to manually unpack from the 'app.asar' because node could not reference yarn to correctly spawn. This then needed some path correction.

Screenshots/GIFs: N/A

@Jtfinlay
Copy link
Author

Jtfinlay commented Sep 9, 2018

Hmm, I am hitting an issue mac-only when I try to run a project task where it spits out:

Yarn requires Node.js 4.0 or higher to be installed.
Task failed

Works fine on Windows. Seeing whether its from these changes, machine's on node v8.11.4

Edit: Also seeing a problem with some of the assets expected by electron.js not being copied by webpack. updating title to WIP and will hunt down these bugs

@Jtfinlay Jtfinlay changed the title #26 Move to electron-builder WIP: #26 Move to electron-builder Sep 9, 2018
@superhawk610
Copy link
Collaborator

I remember that same error message coming up in #125, but I'm not at my PC right now to find exactly where.

Copy link
Collaborator

@AWolf81 AWolf81 left a comment

Choose a reason for hiding this comment

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

Thanks for your work. Looks good to me - just smaller modifications open & the fix for the error on Mac.
I think the main problem we had before was the unpacking inside the electron-builder config that you've added.

I'll have a look if I'm getting Yarn requires Node.js 4.0 ... error on Ubuntu.
I'll also test it on Windows.

package.json Outdated
@@ -73,7 +78,7 @@
"dotenv": "5.0.0",
"dotenv-expand": "4.2.0",
"electron": "2.0.1",
"electron-packager": "12.1.0",
"electron-builder": "^20.28.4",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: We're pinning versions. Please remove the caret.

@@ -39,7 +39,7 @@ export const PACKAGE_MANAGER_CMD = path.join(
remote.app.getAppPath(),
'node_modules/yarn/bin',
formatCommandForPlatform(PACKAGE_MANAGER)
);
).replace('app.asar', 'app.asar.unpacked');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this required? Can you please give some details?
What was the error message if you're not replacing it?

Copy link
Author

Choose a reason for hiding this comment

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

Once unpacking yarn, it is placed under the directory \app.asar.unpacked\node_modules.. instead of within the \app.asar\node_modules.. asar file. Even though it was unpacked, code is still referencing it within the app.asar, so replacing the path.

Same issue is discussed here. I elected to workaround since this was the only case I knew of, but we could instead use the hazardous package referenced in the discussion if desired.

@Jtfinlay
Copy link
Author

Assets are coming in properly now, but haven't solved the 'Yarn requires Node.js 4.0 or higher to be installed.' problem yet. yarn start works fine still, just the bundled version is hitting it.

Copy link
Owner

@joshwcomeau joshwcomeau left a comment

Choose a reason for hiding this comment

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

First, thanks so much for tackling this issue, @Jtfinlay! It's super important, our highest priority right now.

It seems like some of our issues have to do with asar, and I'm wondering if we can skirt them by disabling asar entirely? As I understand it, asar is some tool to make it so package contents can't be inspected, but I have no issue if people want to dig around inside Guppy (in fact I'd encourage it!)


When I cloned your fork and ran yarn start, I'm met with the following error:

screen shot 2018-09-10 at 8 25 57 am


Finally, I believe I'm seeing the error you're aware of; when building a Mac copy, I'm able to start the app, but during project creation, it stalls on "Installing build tool" with the following error:

Uncaught Error: spawn npx ENOENT
    at _errnoException (util.js:1024)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:190)
    at onErrorNT (internal/child_process.js:372)
    at _combinedTickCallback (internal/process/next_tick.js:138)
    at process._tickCallback (internal/process/next_tick.js:180)

This is the error given when no Node installation can be found (apologies for how inscrutable that error is; we're working on falling back to using built-in Node to avoid this situation).

Let me know if I can do anything to help!

output: {
// The build folder.
path: paths.appBuild,
// Generated JS file names (with nested folders).
// There will be one main bundle, and one file per asynchronous chunk.
// We don't currently advertise code splitting but Webpack supports it.
filename: 'static/js/[name].[chunkhash:8].js',
chunkFilename: 'static/js/[name].[chunkhash:8].chunk.js',
Copy link
Owner

Choose a reason for hiding this comment

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

I believe the hashes are added so that Chrome knows when to invalidate its cache of the JS bundles, right?

Right now that's fine since this file never changes, but presumably once we add the ability to update, we'll need to know when the file's changed.

What was the reason for this change? I suspect I'm misunderstanding something haha.

Copy link
Author

Choose a reason for hiding this comment

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

Hmm, I hadn't considered whether the updater uses the hash to update changed files as well. I made this change under assumption that it wasn't needed for electron, but if it is needed then that makes this difficult.

I did this because the package.json has to reference the entry script (electron.js). Since the file is generated by webpack, it breaks the static reference if there's a dynamic chunk name as part of the file path.

Copy link
Owner

Choose a reason for hiding this comment

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

Ahh gotcha. Ok, let's leave it as-is for now. If that does become an issue, it's one we can address later on (really we should disable Chrome's browser cache altogether... and it might already be)

src/electron.js Outdated Show resolved Hide resolved
@mathieudutour
Copy link
Collaborator

As I understand it, asar is some tool to make it so package contents can't be inspected

Not only. It also makes sure that the paths are not breaking on Windows: deeply nested files (like in node_modules for example) just don't work on Windows so having a archive fixes that

@joshwcomeau
Copy link
Owner

joshwcomeau commented Sep 10, 2018

Not only. It also makes sure that the paths are not breaking on Windows: deeply nested files (like in node_modules for example) just don't work on Windows so having a archive fixes that

Ah, that's good to know.. but we shouldn't need deeply-nested paths, right? Especially now that we have a Webpack build for electron.js, it should just be a couple bundles that it needs.

EDIT: But yeah, maybe your point was that it does a whole bunch of small things like that, and removes a lot of potential headaches?

Given that 0.2 works on Windows without asar, I still think I'd like to try without it, unless there are presently more problems without it than with it.

@mathieudutour
Copy link
Collaborator

possibly yes. Just wanted to point it out in case it causes some weird bug in the future ;)

@Jtfinlay
Copy link
Author

Jtfinlay commented Sep 10, 2018

I will try it without asar tonight and at least we can see if it solves the issue. I won't be able to do so until this evening, so anyone can feel free to try given the high priority.

When I cloned your fork and ran yarn start, I'm met with the following error:

This is because of the way I'm referencing 'build/' in package.json. It can't find the file if build hasn't run. I'll see what I can do to keep it consistent between src and build.

@joshwcomeau
Copy link
Owner

Ahh thanks for the clarification @Jtfinlay, makes sense!


// In production, we need to use `fixPath` to let Guppy use NPM.
// For reasons unknown, the opposite is true in development; adding this breaks
// everything.
if (process.env.NODE_ENV !== 'development') {
icon256Path = '../build/256x256.png';
Copy link
Author

Choose a reason for hiding this comment

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

I'm not thrilled by this, but I couldn't find an easy way to keep a reference to the image in both yarn run and after build that would satisfy both environments.

@Jtfinlay
Copy link
Author

I've updated from feedback and yarn run should be working now. Node environment is still a problem, but I'm under water on wedding planning at the moment.

@joshwcomeau
Copy link
Owner

Hi @Jtfinlay,

Awesome, thanks for updating!

Did you have the chance to try it without asar? No worries if not, just asking in case it's a dead end.

Hope wedding planning isn't too stressful, I know it can involve a ton of work!

@Jtfinlay
Copy link
Author

I tried without asar but it would then fail on build, didn't look too far into why.

@AWolf81
Copy link
Collaborator

AWolf81 commented Sep 16, 2018

At the moment, I'm looking at the Node env. issue. But I'm not exactly sure what's causing the issue. DevServer runs correctly but build fails with react-scripts not found.
It seems like passing env to spawn is causing the issue. If I remove the env completely it's working. So the issue is not related to electron-builder setup.

One small thing I noticed is that in getBaseProjectEnvironment adding the path.delimiter is not needed because it's already at the end of process.env.path so there are two delimiters e.g. ;; on Windows. But that's not fixing the build issue.

A fix would be to modify getBaseProjectEnvironment function to:

export const getBaseProjectEnvironment = (projectPath: string) => ({
  ...window.process.env,
  NODE: 'C:\\Program Files\\nodejs\\node.exe',
  NODE_EXE: 'C:\\Program Files\\nodejs\\node.exe',
  PATH:
    window.process.env.PATH +
    path.join(projectPath, 'node_modules', '.bin', path.delimiter),
});

If we're adding it we need to add cross-platform support or if it's just Windows related I think it would be OK to just add NODE & NODE_EXE for Windows. I'm not sure why both keys are required but I've tested each combination. I also tested to add NODE_PATH and adding node path to PATH. But both wasn't working.

With the modified code each task & devServer is running in the Windows binary.

@AWolf81
Copy link
Collaborator

AWolf81 commented Sep 19, 2018

Another option with-out adding NODE to env would be to add shell: true to spawn for tasks & launchDevServer. This fixes the node env issue. Build and devServer is starting.

This feels better and is cross-platform. We just need to test task killing again. As this could be affected by adding shell.
@superhawk610 what do you think should we add shell: true again? Are there any drawbacks?

@joshwcomeau
Copy link
Owner

Another option with-out adding NODE to env would be to add shell: true to spawn for tasks & launchDevServer. This fixes the node env issue. Build and devServer is starting.

Right, so the project started with shell: true because there were weird env issues on Mac. The fix for that, IIRC, was just that we needed to specify window.process.env. Bummer to hear that it didn't totally fix it :/

If we want to move to shell: true, the only thing we need to ensure is that we can still have 2-way communication, to send additional commands once the process has started. This is needed now for eject, but even more in the future when we have custom UI for test running.

@AWolf81
Copy link
Collaborator

AWolf81 commented Nov 1, 2018

We can close this PR with-out merging as it was merged with PR #267 to master.

@AWolf81 AWolf81 closed this Nov 1, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants