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

Fix npm install and npm start for Node 12.x #39

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

achekulaev
Copy link

@achekulaev achekulaev commented Dec 5, 2019

npm install currently fails for all modern node verions.
There is no way to make it build for 13.x right now, since node-pty will not build for 13.x, so the most recent version it can build for is Node 12.x.
electron and electron-builder updates needed to support it.
Because of electron updates some code had needs updates to build.

@matthew-matvei
Copy link
Owner

@achekulaev Thanks very much for your input! I'll have a look into this (it's admittedly been a while since updating / building FreeMAN, and I'm having fun with balancing breaking (for me) dependencies).

It looks like I'll have to have a look at why AppVeyor isn't happy with it either.

Quick question: what's the reasoning for using a wildcard (*) for the versions of electron and electron-builder?

@achekulaev
Copy link
Author

achekulaev commented Dec 6, 2019

I find that usually latest electron version works just fine, and cases when it doesn't work just fine are not more often then the cases when old versions don't work fine (like this case for instance). So I just put asterisk for electron and consequently for builder.

I should note that despite these fixes do fix building and starting I could not get Freeman UI to actually work.

@matthew-matvei
Copy link
Owner

@achekulaev

Okay. I would generally prefer a more defined approach to dependency versioning, but that's not a blocker for me.

Looking at the AppVeyor build, it appears to fail at linting. Could you please run npm run lint and npm run test as described in the PULL_REQUEST_TEMPLATE.md? That should point you to some violations.

These mostly seem to be related to handling Promises now that Electron's API has been updated. The only non-trivial issue is the now asynchronous BrowserWindow.loadURL call in the FreemanWindow's constructor. I moved this call out into main/index.ts to sort out to properly await it in my debugging.

That may fix the immediate issue of the build, but I also currently have issues running the application (this PR is somewhat bringing this project out of the archive). That would need to be addressed, but that's not an issue your PR needs to handle.

@achekulaev
Copy link
Author

I have addressed linting errors, but I cannot really get xvfb-maybe electron-mocha --renderer __tests__ to finish and don't have enough knowledge about electron-mocha to address it.

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.

2 participants