apicache icon indicating copy to clipboard operation
apicache copied to clipboard

Support node v13

Open pqvst opened this issue 6 years ago • 7 comments

It would be great if the current latest release of node (v13) wasn't disallowed. What's blocking?

"node": ">=6.2.0 <13"

https://github.com/kwhitley/apicache/blob/master/package.json#L13

pqvst avatar Jan 13 '20 09:01 pqvst

Would love to @pqvst but I didn't have time to track down what's causing the break!

Attention All: Help Wanted

I need some help tracking down what's causing various tests to fail under Node v13! Whoever figures this one out gets my undying gratitude (and obviously mentions in the docs) ❤️

Contributing

  1. Ensure running Node v13+ (e.g. nvm use v13)
  2. Fork repo
  3. npm install
  4. npm test --watch (watch tests fail)
  5. Hopefully isolate the error and [ideally] submit a PR, but hey, I'll take insight...

kwhitley avatar Jan 13 '20 17:01 kwhitley

I spent some time today looking at this, tests are broken on v13.0.0 as well as v13.8.0 (latest at time of writing).

I'm mostly getting timeout errors from Mocha, suggesting that supertest is not resolving. I didn't have any luck hacking around that though (increasing mocha timeout to 30 seconds, manually calling done), and I can get the resolved response from supertest which puzzles me further.

I focused on respects if-none-match header specifically. I could not find any issues open with supertest about timeouts on Node v13, so I'm not sure it's supertest specific.

jonchurch avatar Feb 07 '20 20:02 jonchurch

It might be a problem with superagent. Some of their tests fail in very similar ways to the ones here on v13.

Opened an issue there.

I'm not sure if the above is responsible for all the fails, but wanted to share back what I've learned.

jonchurch avatar Mar 06 '20 23:03 jonchurch

I looked into this a bit... was able to get it (mostly) working by modifying these lines to be:

if (requestEtag && cachedEtag === requestEtag) {
      response.status(304)
      response.set(headers)

      // writehead needed for possible overrides by restify
      response.writeHead(304)
      return response.end()
    }

    response.status(cacheObject.status || 200)
    response.set(headers)

    // writehead needed for possible overrides by restify
    response.writeHead(cacheObject.status || 200)

Not sure why though, it seems like something goes weird with passing the headers in to writeHead.

However, the restify+gzip test suite still fails, it seems like it's trying to parse the gzipped response as JSON, but I'm not entirely sure. Get errors like:

       restify+gzip tests
         properly caches a request:
     SyntaxError: Unexpected token  in JSON at position 0
      at JSON.parse (<anonymous>)
      at IncomingMessage.<anonymous> (node_modules/superagent/lib/node/parsers/json.js:11:35)
      at endReadableNT (_stream_readable.js:1201:12)
      at processTicksAndRejections (internal/process/task_queues.js:84:21)

pbeshai avatar Apr 23 '20 20:04 pbeshai

Since EOL for 13.x was 2020-06-01, I suggest aiming for the current stable 14.x instead.

rigelk avatar Aug 27 '20 08:08 rigelk

I looked into this a bit... was able to get it (mostly) working by modifying these lines to be:

if (requestEtag && cachedEtag === requestEtag) {
      response.status(304)
      response.set(headers)

      // writehead needed for possible overrides by restify
      response.writeHead(304)
      return response.end()
    }

    response.status(cacheObject.status || 200)
    response.set(headers)

    // writehead needed for possible overrides by restify
    response.writeHead(cacheObject.status || 200)

Not sure why though, it seems like something goes weird with passing the headers in to writeHead.

However, the restify+gzip test suite still fails, it seems like it's trying to parse the gzipped response as JSON, but I'm not entirely sure. Get errors like:

       restify+gzip tests
         properly caches a request:
     SyntaxError: Unexpected token  in JSON at position 0
      at JSON.parse (<anonymous>)
      at IncomingMessage.<anonymous> (node_modules/superagent/lib/node/parsers/json.js:11:35)
      at endReadableNT (_stream_readable.js:1201:12)
      at processTicksAndRejections (internal/process/task_queues.js:84:21)

Thanks!

I added the following debug lines to node_modules/superagent/lib/node/parsers/json.js:

   11       console.log('response', res.text)
   12       console.log('end response')
   13       var body = res.text && JSON.parse(res.text);

And got the following output when running properly caches a request test:

response [{"title":"The Prestige","director":"Christopher Nolan"},{"title":"Schindler's List","director":"Steven Spielberg"}]
end response
response U�;
�0Eѭ�il�[[!vb��
               �$L�ډ��g�HY#��)�����A���-]��K��!�5�m?���鈐������)N$�
#nO��y;��t
end response

kontrollanten avatar Dec 08 '20 10:12 kontrollanten

https://github.com/kwhitley/apicache/pull/228 should fix this issue

Chocobozzz avatar Dec 08 '20 10:12 Chocobozzz