Skip to content
This repository has been archived by the owner on Apr 30, 2020. It is now read-only.

Add backend infrastructure for Extras #14

Closed
wants to merge 5 commits into from

Conversation

tilomitra
Copy link
Contributor

This pull request adds the necessary backend infrastructure for the Extras view. Notably it adds:

  • The necessary routing plumbing.
  • A githubApi module responsible for querying with Github. The module only queries for new repos if a certain time threshold has been crossed. It stores the results in memory.
  • extras route handler that has an inProgress flag which checks to see if an API call is in progress, and serves the older out-of-date result set while the new set completes.
  • extras.handlebars view that loops through the returned Github repo information and creates the necessary DOM.

"handlebars" : "1.x"
"handlebars" : "1.x",
"async" : "0.2.x",
"request" : "2.x"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you need me to do a npm shrinkwrap?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah.

@ericf
Copy link
Collaborator

ericf commented Mar 8, 2013

So this looks good!

I would recommend removing "Api" from the github module and file names. I would also only export the two functions. You can creating them as named functions, and hang them off the exports object; we don't always have to use module.exports =. I would also avoid using this in utility functions like this.

I also think the pending request stuff should be handled in the github module, this alleviates the extras route handler from caring.

@tilomitra
Copy link
Contributor Author

Thanks for the feedback! What's wrong with using this in utility functions, btw?

@ericf
Copy link
Collaborator

ericf commented Mar 8, 2013

These utility methods will be called with function invocation, and are likely to have a context apply()d. If we were defining these as methods on an object prototype, then using this is more accurate because the functions will be called via method invocation.

Here's an example of how someone might want to use this GitHub service:

var getRepos = require('../github').getRepos;

module.exports = function (req, res) {
    // Calling `getRepos()` like this will mean it's invoked 
    // within the global context.
    getRepos({
        username: 'ericf',
        reponame: 'yui3'
    }, function (err, repos) {
        res.render('view', {repos: repos});
    });
};

The above is actually what I think we'll end up (require('../github').getRepos) in our route handler and with the way the GitHub service module is setup, it will break since it assumes inside this function that this is the object exported from the module.

@davglass
Copy link

davglass commented Mar 8, 2013

My only concern with this is that it feels a little "over engineered" by making the web page query the Github api for a repo list. Couldn't this just be a startup task? When the process is started, fetch the repo info and write it to the config (if it hadn't changed, could also stat it and compare mtime)? Then it wouldn't even need an oAuth token on the server since it would never hit the API too often.

Seems like it would be more stable to only do that on process start than to keep a long lived timeout that has a potential for throwing (if data is bad, etc).

@ericf
Copy link
Collaborator

ericf commented Mar 14, 2013

Before we decide whether we're going to switch implementation approaches (periodic and cached, or on startup), is this feature still applicable?

@tilomitra
Copy link
Contributor Author

Yeah, it is. It's just lower priority than the rest of the stuff, but still applicable.

@davglass
Copy link

I don't want to see it launched with this extra overhead in the processes on my server. A long lived timeout is not a good thing.

@tilomitra
Copy link
Contributor Author

Ok, I'll re-implement it. No problemo.

@ericf
Copy link
Collaborator

ericf commented Mar 14, 2013

I don't want to see it launched with this extra overhead in the processes on my server. A long lived timeout is not a good thing.

@davglass this would be making something on the order of 10 APIs calls per hour, and act like a cron job.

@davglass
Copy link

But why? It's just pulling new menu items. Technically, if we launch a new
one, why would we not deploy too? Static > dynamic for this smells better.

Dav Glass
davglass@gmail.com
blog.davglass.com

  • Windows: n. - The most successful computer virus, ever. +
  • A computer without a Microsoft operating system is like a dog
    without bricks tied to its head +
  • A Microsoft Certified Systems Engineer is to computing what a
    McDonalds Certified Food Specialist is to fine cuisine +

On Thu, Mar 14, 2013 at 3:08 PM, Eric Ferraiuolo
notifications@github.comwrote:

I don't want to see it launched with this extra overhead in the
processes on my server. A long lived timeout is not a good thing.

@davglass https://github.com/davglass this would be making something on
the order of 10 APIs calls per hour, and act like a cron job.


Reply to this email directly or view it on GitHubhttps://github.com//pull/14#issuecomment-14926075
.

@tilomitra
Copy link
Contributor Author

We probably won't use this anymore. We'll probably go with Bower instead.

@tilomitra tilomitra closed this May 30, 2013
@ericf
Copy link
Collaborator

ericf commented May 31, 2013

We probably won't use this anymore. We'll probably go with Bower instead.

+1

ericf pushed a commit to ericf/pure-site that referenced this pull request Aug 21, 2013
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants