pinojs / pino-http

🌲 high-speed HTTP logger for Node.js
MIT License
527 stars 117 forks source link

Updating for v9: update dependencies and CI #300

Closed Ranieri93 closed 11 months ago

Ranieri93 commented 11 months ago

Hello everyone!

This PR is referencing this issue.

Quick question, should I leave "pino-std-serializers": "^6.0.0" like this? Or do we expect a major release after the update for v9?

Thank you in advance!

@jsumners

jsumners commented 11 months ago

Quick question, should I leave "pino-std-serializers": "^6.0.0" like this? Or do we expect a major release after the update for v9?

We will not be able to release new versions of ecosystem modules until pino@9 is ready to be released. The updated ecosystem modules will be remain on their respective next branches until that time (see https://github.com/orgs/pinojs/projects/1). Any modules that have dependencies on each other should use git URLs to specify the dependencies until we are ready to publish actual versions.

Ranieri93 commented 11 months ago

Quick question, should I leave "pino-std-serializers": "^6.0.0" like this? Or do we expect a major release after the update for v9?

We will not be able to release new versions of ecosystem modules until pino@9 is ready to be released. The updated ecosystem modules will be remain on their respective next branches until that time (see https://github.com/orgs/pinojs/projects/1). Any modules that have dependencies on each other should use git URLs to specify the dependencies until we are ready to publish actual versions.

I never used such a syntax, I researched a little bit and I hope this is the correct one, but correct me if I'm wrong!

Also, this is probably a stupid question but: I'm seeing tests failing on node 20 because the coverage threshold is not met, what should be done in this case? Because test response comes always with different uncovered line numbers, I'm a bit confused!

Thank you in advance as always for your response!

jsumners commented 11 months ago

The primary issue is that this test is failing under Node 20: https://github.com/pinojs/pino-http/blob/9bf08025852013b5927b5b275ec5bc23fb38a015/test/test.js#L477-L517

It's not clear to me why. I reduced the issue down to the following script that works correctly under Node 18 but returns a 400 response under node 20:

'use strict'

const http = require('http')
const net = require('net')

const server = http.createServer(function (req, res) {
  res.statusCode = 200
  res.end('ok')
})

server.listen(0, '127.0.0.1', function (err) {
  if (err) {
    console.error(err)
    server.close()
    return
  }

  const client = net.connect(server.address().port, '127.0.0.1', () => {
    client.write('GET /delayed HTTP/1.1\r\n\r\n')
  })

  client.on('data', (data) => {
    console.log('data: ', data.toString())
    client.destroy()
    server.close()
  })
})

@mcollina any ideas here?

jsumners commented 11 months ago

@Ranieri93 you should be able to rebase now to get passing tests.