Add GitHub auto-updater by jman294 · Pull Request #33 · cdnjs/autoupdate

jman294 · 2018-04-27T17:20:12Z

The git updater without cloning individual repositories.

PeterDaveHello

@jman294 thanks for the PR, please squash the commits into a single one first, thanks!

PeterDaveHello · 2018-04-27T18:02:24Z


 var update = function(library, callback) {
  var target = library.autoupdate.target;
+  console.log(target)


Should git auto-update be touched here?

PeterDaveHello

Can we set the user-agent in a single place?

PeterDaveHello · 2018-04-27T18:02:50Z

+var update = function(library, callback) {
+  async.series([
+    function(next) {
+      rp({uri:'https://api.github.com/repos/'+githubRepo(library.autoupdate.target)+'/tags',


let's give + spaces between and after it

PeterDaveHello · 2018-04-27T18:03:02Z

+    function(next) {
+      rp({uri:'https://api.github.com/repos/'+githubRepo(library.autoupdate.target)+'/tags',
+        headers: {
+          'User-Agent': 'cdnjs'


Let's say CDNJS GitHub auto-updater

PeterDaveHello · 2018-04-27T18:03:31Z

+            var basePath = library.autoupdate.basePath || "";
+            var allFiles = [];
+
+            rp({uri:'https://api.github.com/repos/'+githubRepo(library.autoupdate.target)+'/git/trees/'+fullData.tree+'?recursive=1',


let's give + spaces between and after it

PeterDaveHello · 2018-04-27T18:03:46Z

+
+            rp({uri:'https://api.github.com/repos/'+githubRepo(library.autoupdate.target)+'/git/trees/'+fullData.tree+'?recursive=1',
+              headers: {
+                'User-Agent': 'cdnjs'


Let's say CDNJS GitHub auto-updater

PeterDaveHello · 2018-04-27T18:04:06Z

+                fs.writeFileSync(libraryPath, JSON.stringify(libraryJSON, undefined, 2) + '\n');
+              }
+              async.each(allFiles, function(file, callback) {
+                //var fileName = path.relative(path.join(localTarget, file.basePath), file._);


Should be removed?

jman294 · 2018-04-27T20:11:36Z

Sorry for somewhat unprofessional style, squashed all commits into one (I believe).

PeterDaveHello · 2018-04-28T06:41:00Z

 tmp
 git_repo_local_cache/
+
+*.swp


We shouldn't update .gitignore in this PR, it's not related. I also suggest to include it in your personal global gitignore list, like:
https://github.com/PeterDaveHello/Unitial/blob/master/gitignore_global#L2
https://github.com/PeterDaveHello/Unitial/blob/master/gitconfig#L72

PeterDaveHello · 2018-04-28T06:42:07Z

+};
+
+var githubRepo = function (url) {
+  return url.slice(url.indexOf('/', 10)+1, url.indexOf('.git'))


missing spaces for +

Are there any other stylistic changes required? I am happy to make all changes, I just am not aware of your style guidelines. Thanks!

I should copy this one here: https://github.com/cdnjs/cdnjs/blob/master/.jscsrc
We have an issue with it: #12
Thanks for asking!

jman294 · 2018-04-28T15:00:31Z

+            .then(function (jsonString) {
+              var filesJson = JSON.parse(jsonString);
+              _.each(library.autoupdate.fileMap, function(mapGroup) {
+                var cBasePath = mapGroup.basePath || "", files = [];


Should use single quotes here?

I bet you're right!

jman294 · 2018-04-28T15:02:18Z

+                    rp({
+                      uri:'https://raw.githubusercontent.com/'
+                        + githubRepo(library.autoupdate.target) + '/'
+                        + file.tree + '/' + file.basePath + '/' + fileName,


Could change this to path.normalize(path.join(file.tree, file.basePath, fileName)),

This is a path for a website, but I think the benefit still applies.

Sounds good!

PeterDaveHello · 2018-04-29T09:39:50Z

Hi @jman294! I've added Travis CI to help check coding style issues, hope it'll help us save time on style issues, please take a look, thank you! 😄

Download correct tarballs Add file retrieval functionality Fix up github updater Delete .cdnjs.js.swp Delete .git.js.swp Delete .autoupdate.js.swp Fixed up style Issue 27 GitHub Updater Add space after + Github Updater Fix style

jman294 · 2018-05-01T01:30:13Z

Is there anything that needs to happen for this pull request to complete? I have tested the updater to the best of my ability.

PeterDaveHello

I'd like to invite @extend1994 & @sufuf3 join the review and test here.

extend1994 · 2018-07-18T07:13:01Z

Review in progress, might need some more time.

extend1994

Seems like this PR can't be used to get the assets in the release page which #27 hopes to achieve?
The used GitHub API in this PR is to get files in a Git repository, #27 wants us to use https://developer.github.com/v3/repos/releases/#releases in my view.

extend1994 · 2018-07-22T01:09:46Z

+          });
+
+          var needed = _.filter(versions, function (version) {
+            var tagName = versions[0].version;


version.version[0] should be the one to retrieve a tag name :)

extend1994 · 2018-07-22T01:11:06Z

+          var needed = _.filter(versions, function (version) {
+            var tagName = versions[0].version;
+            if ((tagName === 'v' || tagName === 'V' || tagName === 'r') &&
+                  version.length > 1 && !isNaN(version[1])) {


Here we want to check version length and numberic version, so it would be version.version.length > 1 && !isNaN(version.version[1])according to your version object.

extend1994 · 2018-07-22T01:13:17Z

+                _.each(mapGroup.files, function (cRule) {
+                  var newFiles = [];
+                  for (var file of filesJson.tree) {
+                    if (file.path.indexOf(cBasePath) === 0) {


Let's add && file.type !== "tree" here in order to prevent directory match like what option nodir: true can do in glob library.

extend1994 · 2018-07-22T01:15:21Z

+                      if (minimatch(file.path, cRule, {
+                        nodir: true,
+                        realpath: true
+                      })) {


Let's remove nodir and realpath option, they are only for glob lib and there has no effect in minimatch lib.
Then the code block here can be simpler:

if (minimatch(file.path, cRule)) { newFiles.push({ path: file.path, tree: fullData.tree }); }

extend1994 · 2018-07-22T01:16:54Z

+                    rp({
+                      uri: 'https://raw.githubusercontent.com/'
+                        + githubRepo(library.autoupdate.target) + '/'
+                        + path.normalize(path.join(file.tree, file.basePath, fileName)),


path.normalize(path.join(file.tree, fileName) is enough, fileName has included basePath.

extend1994 · 2018-07-22T01:22:03Z

+                var fileName = file._;
+                var fileTarget = path.normalize(
+                                    path.join(__dirname, '../../cdnjs', 'ajax',
+                                      'libs', library.name, tag, fileName)


fileName here should be path.relative(file.basePath, fileName);,
I suppose a better way is to replace it with a local and meaningful variable
(instead of using path.relative(file.basePath, fileName); directly)
e.g. Add

var localFileName = path.relative(file.basePath, file._);

under fileName declaration. And use it to replace fileName.

jman294 mentioned this pull request Apr 27, 2018

GitHub auto-updater #27

Open

PeterDaveHello suggested changes Apr 27, 2018

View reviewed changes

PeterDaveHello reviewed Apr 27, 2018

View reviewed changes

PeterDaveHello suggested changes Apr 27, 2018

View reviewed changes

jman294 force-pushed the master branch from 295a0a5 to 237b6b6 Compare April 27, 2018 20:09

PeterDaveHello suggested changes Apr 28, 2018

View reviewed changes

jman294 commented Apr 28, 2018

View reviewed changes

PeterDaveHello force-pushed the master branch from 237b6b6 to cb34627 Compare April 29, 2018 09:34

jman294 force-pushed the master branch from cb34627 to 861f643 Compare April 29, 2018 13:41

Add github updater

27b88f1

Download correct tarballs Add file retrieval functionality Fix up github updater Delete .cdnjs.js.swp Delete .git.js.swp Delete .autoupdate.js.swp Fixed up style Issue 27 GitHub Updater Add space after + Github Updater Fix style

jman294 force-pushed the master branch from 861f643 to 27b88f1 Compare April 29, 2018 14:07

PeterDaveHello reviewed May 3, 2018

View reviewed changes

PeterDaveHello requested review from extend1994 and sufuf3 May 3, 2018 16:02

PeterDaveHello changed the title ~~Github Updater~~ Add GitHub Updater May 3, 2018

PeterDaveHello changed the title ~~Add GitHub Updater~~ Add GitHub auto-updater May 3, 2018

extend1994 suggested changes Jul 22, 2018

View reviewed changes

xtuc force-pushed the master branch from edf7849 to fac1cae Compare January 31, 2020 17:14

Uh oh!

Conversation

jman294 commented Apr 27, 2018

Uh oh!

PeterDaveHello left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PeterDaveHello left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jman294 commented Apr 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jman294 Apr 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PeterDaveHello commented Apr 29, 2018

Uh oh!

jman294 commented May 1, 2018

Uh oh!

PeterDaveHello left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

extend1994 commented Jul 18, 2018

Uh oh!

extend1994 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jman294 Apr 28, 2018 •

edited

Loading

PeterDaveHello left a comment •

edited

Loading

extend1994 left a comment •

edited

Loading