Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

electron-forge install #41

Merged
merged 19 commits into from
Dec 31, 2016
Merged

electron-forge install #41

merged 19 commits into from
Dec 31, 2016

Conversation

MarshallOfSound
Copy link
Member

@MarshallOfSound MarshallOfSound commented Dec 28, 2016

This is a WIP for installing Electron apps from GitHub releases. Currently tested / working / implemented on macOS and theoretically works on Windows (don't have a windows machine atm).

Need some help getting it working on linux (@malept)?

Example:
screen shot 2016-12-28 at 4 33 13 pm

@malept
Copy link
Member

malept commented Dec 28, 2016

My thoughts on installing Linux packages:

  1. If flatpak is installed, use that
  2. If yum is installed, install the RPM
  3. If gdebi is installed, install the .deb
  4. If there's a zip file, install that (I'm not entirely sure where to put it though)

@MarshallOfSound
Copy link
Member Author

MarshallOfSound commented Dec 28, 2016

@malept Currently this PR installs the only target available (if there is only one) or allows the user to choose.

For instance on macOS it will allow the user to choose between .zip and .dmg if both exist.

Are you suggesting we should be defaulting to a certain heirachy of package types and install the highest one that is compatible?

@malept
Copy link
Member

malept commented Dec 28, 2016

I was describing an autodetect method - I wasn't aware of the prompt-based method. That should be fine for Linux, provided we can describe what the packages are.

targetAsset = possibleAssets.find(asset => asset.id === assetID);
}

const tmpdir = path.resolve(os.tmpdir(), 'forge-install');
Copy link
Member

Choose a reason for hiding this comment

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

This folder should probably be removed when the command completes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably, but it's in the tmp directory so the OS will wipe it when it needs to and if we leave it there reinstalls are a million times faster. Idm either way, but it was a godsend during development 😆

@@ -64,14 +66,18 @@
"debug": "^2.3.3",
"electron-installer-dmg": "^0.1.2",
"electron-packager": "^8.4.0",
"electron-sudo": "malept/electron-sudo#fix-linux-sudo-detection",
Copy link
Member

Choose a reason for hiding this comment

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

Just FYI, this can be changed to a version once automation-stack/electron-sudo#41 is merged & released.

"electron-winstaller": "^2.5.0",
"fs-promise": "^1.0.0",
"github": "^7.2.0",
"glob": "^7.1.1",
"inquirer": "^2.0.0",
"lodash.template": "^4.4.0",
"log-symbols": "^1.0.2",
"node-fetch": "^1.6.3",
Copy link
Member

Choose a reason for hiding this comment

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

It feels really weird to add two different modules that perform HTTP requests, even though I know that they're serving two different functions.

Copy link
Member Author

Choose a reason for hiding this comment

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

nugget is just for the fancy UI, I could remove it but I think the feedback is good for such a big download.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I kind of wish nugget was built on fetch, or we used some fancy feedback module that was built on fetch. It's not that important.


const installActions = {
win32: {
'.exe': async filePath => await opn(filePath, { wait: false }),
Copy link
Member

Choose a reason for hiding this comment

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

Since every other installer is one line (except for the darwin zip installer), should this also go in a separate file?

}

const tmpdir = path.resolve(os.tmpdir(), 'forge-install');
const pathSafeRepo = repo.replace(/\//g, '-').replace(/\\/g, '-');
Copy link
Member

Choose a reason for hiding this comment

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

That could be one .replace call.

const GITHUB_API = 'https://api.github.com';

const main = async () => {
const searchSpinner = ora.ora('Searching for Application').start();
Copy link
Member

Choose a reason for hiding this comment

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

I think this hasn't been promisified yet.

@malept malept changed the title [WIP] electron-forge install electron-forge install Dec 31, 2016
@MarshallOfSound MarshallOfSound merged commit 1255803 into master Dec 31, 2016
@MarshallOfSound MarshallOfSound deleted the install-cmd branch December 31, 2016 22:57
dsanders11 pushed a commit that referenced this pull request Jan 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants