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

fix last-modified support #6

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

secondfolder
Copy link

The when making a request the 4chan api does not look to see if the last-modified header in the request is greater than the timestamp of the last change. Instead it just checks to see if they're equal. Thus if we want to make use of the last-modified header we have to pass it the exact value it returned in the header last time we made a request. This patch gives a way to pass that last-modified timestamp and to have it returned without breaking old code.

commit message:

  • Accept and honour last-modified header field
    subapi.thread() now accepts another argument, lastModified. Previously
    thread() returned an array of posts, now it returns an object with the
    same array of posts under the property "posts", along slide the last
    modified timestamp ("lastModified") and status field ("status"). In
    order not to break backwards compatability the plain array of posts
    is returned if no lastModified argument is given.

  • Throw error on 404
    If either subapi.thread() or api.board() encounters a 404 it will
    now throw an error. Previously it returned undefined.

  • Update board url
    4chan api route for a thread is now:
    http(s)://a.4cdn.org/[board]/thread/[threadnumber].json
    Instead of:
    http(s)://a.4cdn.org/[board]/res/[threadnumber].json

- Accept and honor last-modified header field
  subapi.thread() now accepts another argument, lastModified. Previously
  thread() returned an array of posts, now it returns an object with the
  same array of posts under the property "posts", along slide the last
  modified timestamp ("lastModified") and status field ("status"). In
  order not to break backwards compatability the plain array of posts
  is returned if no lastModified argument is given.

- Throw error on 404
  If either subapi.thread() or api.board() encounters a 404 it will
  now throw an error. Previously it returned undefined.

- Update board url
  4chan api route for a thread is now:
  http(s)://a.4cdn.org/[board]/thread/[threadnumber].json
  Instead of:
  http(s)://a.4cdn.org/[board]/res/[threadnumber].json
Copy link
Owner

@yocontra yocontra left a comment

Choose a reason for hiding this comment

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

I agree we need the functionality to be improved, but I'm not sure this is the right solution.

I think it might be simpler to do this by either allowing the user of the module to give a last modified time (for all methods, not just threads), or to do this globally using some kind of cache (so we know the last request time, and can use that as the last modified time).

The error handling behavior should also be applied globally, if any request returns status > 400 it should return an error for that code.

cb(null, body.posts);
});

return api;
};

function threadChanges(num, lastModified, cb) {
console.log(arguments)
Copy link
Owner

Choose a reason for hiding this comment

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

Left a log

console.log(arguments)
var uri = [baseUrl, board, "thread", num+".json"].join("/");

requestOptionsLocal = JSON.parse(JSON.stringify(requestOptions)); // clone global request options
Copy link
Owner

Choose a reason for hiding this comment

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

missing var declaration?

@@ -9,11 +9,11 @@ describe('4chan api', function() {
it('should return a list of boards', function(done) {
this.timeout(5000);
api.boards(function(err, boards){
should.not.exist(err);
Copy link
Owner

Choose a reason for hiding this comment

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

can you remove these whitespace changes? this project uses 2 space tabs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants