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

Updating for v9: update dependencies and CI #300

Merged
merged 2 commits into from
Oct 13, 2023
Merged

Updating for v9: update dependencies and CI #300

merged 2 commits into from
Oct 13, 2023

Conversation

Ranieri93
Copy link
Contributor

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

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

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

The primary issue is that this test is failing under Node 20:

pino-http/test/test.js

Lines 477 to 517 in 9bf0802

test('log requests aborted during payload', function (t) {
const dest = split(JSON.parse)
const logger = pinoHttp(dest)
function handle (req, res) {
logger(req, res)
const read = new stream.Readable({
read () {
if (this.called) {
return
}
this.called = true
this.push('delayed')
}
})
read.pipe(res)
}
function listen (err, server) {
t.error(err)
const client = net.connect(server.address().port, server.address().address, () => {
client.write('GET /delayed HTTP/1.1\r\n\r\n')
})
client.on('data', (data) => {
client.destroy()
})
}
setup(t, logger, listen, handle)
dest.on('data', function (line) {
t.ok(line.responseTime >= 0, 'responseTime is defined')
t.equal(line.msg, DEFAULT_REQUEST_ABORTED_MSG, 'message is set')
t.end()
})
})

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 jsumners mentioned this pull request Oct 11, 2023
@jsumners
Copy link
Member

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

Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@jsumners jsumners merged commit 008ee8a into pinojs:next Oct 13, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants