-
Notifications
You must be signed in to change notification settings - Fork 45
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
Move common functionality to electron-installer-common #94
Conversation
Also, require Node 6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is some great work!!! I've left some comments about things that need changing to bring both -debian
and -redhat
closer together. None of those are required changes to this PR, they can be submitted in a different one
license: pkg.license, | ||
|
||
group: undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a reminder that we need to remove the group
option. It's been deprecated since Fedora 18 and it's ignored now.
group: undefined, | ||
|
||
arch: undefined, | ||
|
||
requires: [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A reminder that this list of dependencies needs to be fixed. Maybe moved to the common module since both -debian
and -redhat
have the same logic on deciding which dependencies to use based on electron's version. It's just the names that differ. Or just add them as part of the private utility functions of each module.
I like the idea of moving it to the -common
, so we can handle any dependency change in one place only
compressionLevel: 2, | ||
bin: pkg.name || 'electron', | ||
execArguments: [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why we support this on -redhat
only. This is common to both. We'll need to make some changes to desktop.ejs
for -debian
, and add a description to the README there too.
@@ -360,8 +185,7 @@ module.exports = function (data, callback) { | |||
options.version = adjustedVersion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A reminder that all this logic should be moved to be part of the getDefaults function. Similar to how we make it on -debian
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I also want to expose the version adjusting code in the same manner as -debian
so I can use it in Electron Forge.
Also, require Node 6.
Similar to electron-userland/electron-installer-debian#162
Incidentally, the PR #91 was what prompted me to finally finish the first iteration of the module plus these PRs, since I've already done a chunk of the custom desktop template work (by virtue of copying it from
-debian
).