-
-
Notifications
You must be signed in to change notification settings - Fork 29
Consolidate electron-download options #320
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
Conversation
131e0ae to
412d53f
Compare
412d53f to
cd3f488
Compare
|
This is in a better spot to be reviewed now, using the normal UI. |
| 'download.strictSSL' | ||
| ], | ||
| default: { | ||
| 'strict-ssl': true, |
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 actually fixes a bug with strict-ssl 😢
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 should probably use the alias feature of minimist, so the two values are always in sync. Otherwise, the other default might overwrite the user setting.
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, wow. Good catch. This means that users using electron-packager v6 from the command line will accept invalid ssl certificates.
This is bad enough that I would recommend running npm deprecate on all older versions so users know to upgrade.
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.
Unclear.
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 should probably use the
aliasfeature of minimist, so the two values are always in sync. Otherwise, the other default might overwrite the user setting.
Hmm. I'll have to look into that, thanks.
bc35b61 to
ebea1d8
Compare
| 'download.strictSSL' | ||
| ], | ||
| default: { | ||
| 'strict-ssl': true, |
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.
All versions or just 6.x?
Consolidates
cacheandstrict-sslinto the newdownloadparameter, which is more or less passed directly toelectron-download(similar toosx-signandversion-string). The original two parameters are now deprecated. This also lets us easily add sub-properties such asmirror(as requested in #316).Depends on #319
, so this might be a better diff to look at until it's merged.Closes #316.