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

Support node v13 #189

Open
pqvst opened this issue Jan 13, 2020 · 7 comments
Open

Support node v13 #189

pqvst opened this issue Jan 13, 2020 · 7 comments

Comments

@pqvst
Copy link

pqvst commented Jan 13, 2020

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

@kwhitley
Copy link
Owner

kwhitley commented Jan 13, 2020

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...

@jonchurch
Copy link

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
Copy link

jonchurch commented Mar 6, 2020

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.

@pbeshai
Copy link

pbeshai commented Apr 23, 2020

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)

@rigelk
Copy link

rigelk commented Aug 27, 2020

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

@kontrollanten
Copy link
Contributor

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

@Chocobozzz
Copy link
Contributor

#228 should fix this issue

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

No branches or pull requests

7 participants