Skip to content

Conversation

@evanshortiss
Copy link

Currently the module only caches on the use of res.send and has issues with binary files and JSON. This PR should help resolve such issues, but might need a little more work.

Tests now use supertest rather than a spawned process, I found this easier to work with. Let me know your thoughts!

Caching occurs on data passed to res.socket.write, not res.send now. This is is the main update that facilitates support for res.json and binary responses. Cached items now represent the data sent to the response stream too, so this is what gets written to the cache:

HTTP/1.1 200 OK
X-Powered-By: Express
Content-Type: text/html; charset=utf-8
Content-Length: 46
ETag: W/"2e-PQ7Dm+NYwzgkRT8czBTdcQ"
Date: Thu, 26 May 2016 03:26:57 GMT
Connection: keep-alive

Now is Wed May 25 2016 22:26:57 GMT-0500 (CDT)

One potential issue is the Date header is cached; we might need to work around this?

@evanshortiss
Copy link
Author

I believe this will fix:

The build is failing, but I can't see why. Should I be able to see the output? They pass locally so I'd need the Travis output to figure it out.

if (!err) {
/** Create the new cache **/
self.add(name, buf, {
type: res._headers['content-type'],
Copy link
Author

Choose a reason for hiding this comment

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

This might not be required now, since the content-type is stored in the body

@evanshortiss
Copy link
Author

I created a new set of module to handle this since I needed more features, it's express-expeditious.

Will leave this PR open in the event it is ever merged 👍

@rv-kip
Copy link
Owner

rv-kip commented Mar 20, 2017

@evanshortiss, can you rebase and resolve conflicts?

@krazyjakee
Copy link
Collaborator

@evanshortiss I understand this is old as hell but can you redo this PR for the dev branch? It would be greatly appreciated.

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.

3 participants