-
Notifications
You must be signed in to change notification settings - Fork 0
Fetch from stackoverflow API #1
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
Fetches lists of questions from SO. Had a look at the PHP room's code but didn't really like it so I mostly ignored it. My code does a lot of looping and I tried to tone it down but it's still a bit messy in the name of having a clear way of seeing which questions are included in what categories and keeping the number of SO API calls down to one. I haven't styled it, I hate CSS. There's basically no error handling.
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| var APP = APP || {}; | ||
| (function(app) { | ||
|
|
||
| var canonicals = { | ||
|
|
||
| closures: [750486], | ||
| asynchronous: [14220321], | ||
| inheritance: [11072556, 7486825] | ||
|
|
||
| }; | ||
|
|
||
| var ids = Object.keys(canonicals).map(function(category) { | ||
| return canonicals[category].join(';'); | ||
| }); | ||
|
|
||
| app.xhr.fetch(ids.join(';')).then(function success(questions) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use |
||
| questions.items.forEach(function(question) { | ||
| Object.keys(canonicals).forEach(function(category) { | ||
| canonicals[category].forEach(function(q, i) { | ||
| //they see me nesting... | ||
| if (q === question.question_id) { | ||
| canonicals[category][i] = question; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wouldn't mutate the existing canonicals array (it kind of beats the point of using higher order functions like .forEach and .map), you have no guarantee for the order in which things run. Creating a new Object with the resolved question objects is likely better.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can do. I was debating with myself if I should be mutating the lists or not and in the end lazyness prevailed. You're right though, it's not that good an idea. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. REDUUUUUUUUCE
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm going to need a crash course on using reduce, will update pull request shortly but I somehow feel I'm not Doing It Right. I ended up doing the same amount of nested loops while increasing the number of LOC. I also didn't quite get how reduce would (pun!) reduce the amount of mapping for building the array of ids sent to the SO API so if you could elaborate that would be very nice. |
||
| } | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| document.body.appendChild(app.dom.h1('List of canonicals')); | ||
|
|
||
| var sections = Object.keys(canonicals).map(function(category) { | ||
| return app.dom.section({ class: 'category' }, [ | ||
| app.dom.h2(category), | ||
| app.dom.ul(canonicals[category].map(function(question) { | ||
| return app.dom.li([createQuestionElement(question)]); | ||
| })) | ||
| ]); | ||
| }); | ||
|
|
||
| document.body.appendChild(app.dom.div({ class: 'canonicals' }, sections)); | ||
| document.body.appendChild(app.dom.div('Number of remaining API calls before meltdown: ' + questions.quota_remaining)); | ||
| }, function error(reasons) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use |
||
| document.body.appendChild(app.dom.div('Something went pearshaped because ' + reasons)); | ||
| }); | ||
|
|
||
| function createQuestionElement(question) { | ||
| return app.dom.div([ | ||
| app.dom.span({ class: 'score' }, question.score), | ||
| app.dom.a({ href: question.link, target: '_blank' }, question.title), | ||
| app.dom.div({ class: 'tags' }, question.tags.map(function(tag) { return app.dom.span(tag) })) | ||
| ]); | ||
| } | ||
|
|
||
| }(APP)); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| var APP = APP || {}; | ||
| (function(app) { | ||
|
|
||
| function element(name) { | ||
| return function() { | ||
| var e = document.createElement(name); | ||
| var args = Array.prototype.slice.call(arguments); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't use the argument array like that, it's not readable.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can change this, but it means having to do stuff like |
||
| args.forEach(function(arg) { | ||
| var type = Object.prototype.toString.call(arg); | ||
| if (type.indexOf('Object') > -1) { | ||
| for (var prop in arg) { | ||
| if (arg.hasOwnProperty(prop)) { | ||
| e.setAttribute(prop, arg[prop]); | ||
| } | ||
| } | ||
| } else if (type.indexOf('Array') > -1) { | ||
| arg.forEach(function(child) { | ||
| e.appendChild(child); | ||
| }); | ||
| } else if (type.indexOf('String') > -1 || type.indexOf('Number') > -1) { | ||
| e.innerHTML = arg; | ||
| } | ||
| }); | ||
| return e; | ||
| } | ||
| } | ||
|
|
||
| app.dom = {}; | ||
| ['h1', 'h2', 'section', 'div', 'ul', 'li', 'span', 'a'].forEach(function(name) { | ||
| app.dom[name] = element(name); | ||
| }); | ||
|
|
||
| }(APP)); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| .canonicals { | ||
| margin-left: 20px; | ||
| } | ||
|
|
||
| .canonicals ul { | ||
| list-style-type: none; | ||
| } | ||
|
|
||
| .canonicals li { | ||
| padding: 20px 0; | ||
| border-bottom: thin solid gray; | ||
| } | ||
|
|
||
| .canonicals .score { | ||
| width: 50px; | ||
| background-color: lightgray; | ||
| padding: 10px; | ||
| margin: 5px; | ||
| } | ||
|
|
||
| .canonicals .tags { | ||
| margin-top: 20px; | ||
| } | ||
|
|
||
| .canonicals .tags span { | ||
| padding: 5px; | ||
| background-color: lightgray; | ||
| margin: 0 5px 0 5px; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| var APP = APP || {}; | ||
| (function(app) { | ||
|
|
||
| var API_URLROOT = 'https://api.stackexchange.com/2.2/questions/'; | ||
|
|
||
| app.xhr = {}; | ||
|
|
||
| app.xhr.fetch = function(ids) { | ||
| return new Promise(function(resolve, reject) { | ||
| var req = new XMLHttpRequest(); | ||
| var url = app.xhr.makeApiUrl(ids); | ||
| req.onreadystatechange = function() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can do. I didn't know about that and it looks much cleaner. I guess there's no reason to care about IE9 users (I'm probably already throwing Errors for IE9 btw), http://caniuse.com/#feat=xhr2, I might put in some feature-detection and laugh at them if they try to use the page. |
||
| if (req.readyState === 4) { | ||
| if (req.status === 200) { | ||
| return resolve(JSON.parse(req.responseText)); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With the correct type, you can use
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice, I'll look into that. |
||
| } else { | ||
| return reject('shit dun goofed'); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Throw an error There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't throw an error, it's not safe inside the callback, but do wrap the
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But if I do #1 (comment)? and get a JSON responseType I can get rid of the entire JSON.parse call. Question is, what happens then if the SO API returns invalid JSON? I would guess I don't get a 200 status in the first place? |
||
| } | ||
| } | ||
| }; | ||
| req.open('GET', url); | ||
| req.send(); | ||
| }); | ||
| }; | ||
|
|
||
| app.xhr.makeApiUrl = function(questionIds) { | ||
| return API_URLROOT + questionIds + '?site=stackoverflow'; | ||
| }; | ||
|
|
||
| }(APP)); | ||
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.
If you're using modules already, why not just go with a solution like requirejs or commonjs and be done with it? (Not critical though)
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 considered requirejs or browserify, but I think the requirejs syntax is extremely bloated and I don't like it. Browserify is much nicer IMO but would need a build-step so I figured I'd just go bare-bone.
Maybe just scrap the modules entirely and put everything in a single file is just as good? Would save 2 requests as well.
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 for babel :)
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'd love to dig into some ES6 but I think having a build step makes it way more cumbersome to add new question ids to the canonical list. Being able to make a PR with a new question directly through github's web interface increases the chance it will be done dramatically. With a build step PRs also has to include re-transpiled source code.