The Wayback Machine - https://web.archive.org/web/20200912133417/https://github.com/joaomoreno/gulp-atom-electron/pull/59
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

refactor: switch to @electron/get #59

Merged
merged 4 commits into from May 8, 2020

Conversation

@codebytere
Copy link
Contributor

codebytere commented Apr 22, 2020

This PR updates gulp-atom-electron to use Electron's recommended @electron/get for simpler downloading and automatic caching. Some other benefits include the ability to download nightly releases as well as specify custom mirrors.

Tested locally with Electron versions v7.2.1 and v7.2.3 to confirm efficacy of change when running yarn electron in vscode itself.

cc @deepak1556

@deepak1556
Copy link
Collaborator

deepak1556 commented Apr 22, 2020

function download(opts, cb) {
var repo = opts.repo || 'atom/electron';
var github = new GitHub({ repo: repo, token: opts.token });

This comment has been minimized.

@deepak1556

deepak1556 Apr 22, 2020

Collaborator

We still need to support this configuration for the internal builds.

This comment has been minimized.

@deepak1556

deepak1556 Apr 22, 2020

Collaborator

Ah I see that we can specify a mirror, that should do the trick

This comment has been minimized.

@joaomoreno

joaomoreno Apr 22, 2020

Owner

How to do that?

@deepak1556
Copy link
Collaborator

deepak1556 commented Apr 22, 2020

Thanks for working on this @codebytere

@bpasero
Copy link
Contributor

bpasero commented Apr 22, 2020

Cool!

@joaomoreno
Copy link
Owner

joaomoreno commented Apr 22, 2020

@deepak1556 Would you like to drive this PR's adoption in VS Code? I've invited you as a collaborator.

@deepak1556
Copy link
Collaborator

deepak1556 commented Apr 22, 2020

Thanks @joaomoreno , would be happy to do it.

@joaomoreno
Copy link
Owner

joaomoreno commented Apr 22, 2020

Another idea @bpasero and I just discussed about: how about just dropping this? Do we still need this module at all?

@bpasero
Copy link
Contributor

bpasero commented Apr 22, 2020

I was referring to https://github.com/electron-userland/electron-forge in the standup today.

@codebytere
Copy link
Contributor Author

codebytere commented Apr 22, 2020

What do you all envision the alternate path looking like, should we choose to take it? I'm happy to switch up my approach or try to PR something into vscode itself

@codebytere codebytere force-pushed the codebytere:swap-to-electron-get branch from c2bcdf1 to 44ca6ab Apr 22, 2020
@codebytere codebytere force-pushed the codebytere:swap-to-electron-get branch from 44ca6ab to 444e748 Apr 22, 2020
@joaomoreno
Copy link
Owner

joaomoreno commented Apr 23, 2020

@codebytere No, no, this was more of a comment for us, VS Code team.

@codebytere
Copy link
Contributor Author

codebytere commented May 4, 2020

👋 hey folks, is there anything i can do to help move this PR forward this week?

@deepak1556
Copy link
Collaborator

deepak1556 commented May 4, 2020

@codebytere last week was testing in vscode during which the master branch will be frozen, I plan to move this forward tomorrow.

src/download.js Outdated Show resolved Hide resolved
src/download.js Show resolved Hide resolved
src/download.js Outdated Show resolved Hide resolved
src/download.js Outdated Show resolved Hide resolved
src/util.js Outdated Show resolved Hide resolved
src/util.js Outdated Show resolved Hide resolved
src/util.js Outdated Show resolved Hide resolved
@codebytere codebytere force-pushed the codebytere:swap-to-electron-get branch from 27972f5 to cc5b4e9 May 6, 2020
@codebytere codebytere requested a review from deepak1556 May 6, 2020
@deepak1556
Copy link
Collaborator

deepak1556 commented May 6, 2020

Thanks @codebytere for the fast turnaround, the following changes are also needed

diff --git a/src/download.js b/src/download.js
index f408555..5363fdd 100644
--- a/src/download.js
+++ b/src/download.js
@@ -1,7 +1,8 @@
 'use strict';
 
 var path = require('path');
-const { downloadArtifact, getDownloadUrl } = require('@electron/get');
+const { downloadArtifact } = require('@electron/get');
+const { getDownloadUrl } = require('./util')
 var semver = require('semver');
 var rename = require('gulp-rename');
 var es = require('event-stream');
@@ -31,15 +32,20 @@ function download(opts, cb) {
 		version: opts.version,
 		platform: opts.platform,
 		arch,
-		token
+		token: opts.token
 	};
 
 	if (opts.repo) {
 		getDownloadUrl(opts.repo, downloadOptions)
-		.then(({err, downloadUrl}) => {
+		.then(({err, downloadUrl, assetName}) => {
 			if (err) return cb(err)
 	
-			downloadOptions['mirrorOptions'] = { mirror: downloadUrl }
+			downloadOptions['mirrorOptions'] = {
+				mirror: downloadUrl,
+				customFilename: assetName
+			};
+			downloadOptions['artifactName'] = assetName;
+			downloadOptions['unsafelyDisableChecksums'] = true;
 	
 			downloadArtifact(downloadOptions).then(zipFilePath => {
 				return cb(null, zipFilePath)
diff --git a/src/util.js b/src/util.js
index 54499fa..2ace0fb 100644
--- a/src/util.js
+++ b/src/util.js
@@ -7,8 +7,8 @@ function startsWith (haystack, needle) {
 	return haystack.substr(0, needle.length) === needle;
 };
 
-async function getDownloadUrl(repo, { version, platform, arch, token }) {
-	const [owner, repo] = repo.split('/');
+async function getDownloadUrl(repo_url, { version, platform, arch, token }) {
+	const [owner, repo] = repo_url.split('/');
 	const octokit = new Octokit({ auth: token });
 
 	const { data: release } = await octokit.repos.getReleaseByTag({
@@ -46,7 +46,7 @@ async function getDownloadUrl(repo, { version, platform, arch, token }) {
 	});
 
 	const { url, headers } = requestOptions;
-  headers.authorization = `token ${token}`;
+	headers.authorization = `token ${token}`;
 
 	const response = await got(url, {
 		followRedirect: false,
@@ -54,7 +54,7 @@ async function getDownloadUrl(repo, { version, platform, arch, token }) {
 		headers
 	});
 
-	return { error: null, location: response.headers.location };
+	return { error: null, location: response.headers.location, assetName: asset.name };
 }
 
 module.exports = {
@codebytere codebytere force-pushed the codebytere:swap-to-electron-get branch from cc5b4e9 to e942403 May 6, 2020
@deepak1556
Copy link
Collaborator

deepak1556 commented May 6, 2020

features from the previous version are preserved now,

  • Caching
  • downloads from custom repo

There are two issues that still needs addressing

  • the artifact filename in the cache folder has mangled various names together and looks like
    httpsgithub.comelectronelectronreleasesdownloadv7.2.4electron-v7.2.4-darwin-x64.zip when downloaded from custom repo

  • Although the default behavior is to display progress bar only if the download is greater than 30s , which is mostly not the case here as I don't see it in my testing. I would still like some feedback when its less than 30s.

@codebytere codebytere force-pushed the codebytere:swap-to-electron-get branch from e942403 to 97bfded May 6, 2020
@codebytere codebytere force-pushed the codebytere:swap-to-electron-get branch 2 times, most recently from 99a2415 to d9a070a May 6, 2020
Copy link
Collaborator

deepak1556 left a comment

LGTM with minor style changes.

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/download.js Outdated Show resolved Hide resolved
src/download.js Outdated Show resolved Hide resolved
src/download.js Outdated Show resolved Hide resolved
@codebytere codebytere force-pushed the codebytere:swap-to-electron-get branch 2 times, most recently from 9708956 to e582f5e May 7, 2020
@codebytere codebytere force-pushed the codebytere:swap-to-electron-get branch from e582f5e to f3f50e7 May 7, 2020
src/download.js Outdated Show resolved Hide resolved
src/download.js Outdated Show resolved Hide resolved
@codebytere codebytere force-pushed the codebytere:swap-to-electron-get branch from 15302b2 to 1f3bee0 May 8, 2020
@codebytere codebytere requested a review from deepak1556 May 8, 2020
Copy link
Collaborator

deepak1556 left a comment

Thanks for working through my review 😄

Good to go now. @joaomoreno if you can make a new release, will adopt this in VSCode. Thanks!

@deepak1556 deepak1556 merged commit 93e6619 into joaomoreno:master May 8, 2020
@kieferrm kieferrm mentioned this pull request May 10, 2020
45 of 61 tasks complete
deepak1556 added a commit that referenced this pull request Aug 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.