feat: expose method for setGenReqId on FastifyInstance by dancastillo · Pull Request #5259 · fastify/fastify

dancastillo · 2024-01-09T03:29:28Z

Checklist

fixes: #5170

run npm run test and npm run benchmark
tests and/or benchmarks are included
documentation is changed or added
commit message and code follows the Developer's Certification of Origin
and the Code of conduct

metcoder95

Nice work! Left a few comments

test/genReqId.test.js

docs/Reference/Server.md

test/genReqId.test.js

docs/Reference/Server.md

mcollina

lgtm

mcollina · 2024-01-10T15:39:49Z

PTAL @Uzlopak @metcoder95

types/instance.d.ts

Uzlopak

LGTM

I dont know if @metcoder95 s remark is just about missing a unit test, or actually pointing to a potential missing case?!

Also I realized that we could improve the wording of genReqId in Server.md, because my remarks about the wording ("This function is expected to be error-free.") was directly copied by @dancastillo. Maybe we should tackle this in a follow up PR (atleast let us open an issue).

metcoder95

I'm referring to a test like this:

fastify/test/404s.test.js

Lines 722 to 763 in 0be27ca

    
           test('run non-encapsulated plugin hooks on default 404', t => { 
        
             t.plan(6) 
        
             const fastify = Fastify() 
        
             fastify.register(fp(function (instance, options, done) { 
        
               instance.addHook('onRequest', function (req, res, done) { 
        
                 t.pass('onRequest called') 
        
                 done() 
        
               }) 
        
               instance.addHook('preHandler', function (request, reply, done) { 
        
                 t.pass('preHandler called') 
        
                 done() 
        
               }) 
        
               instance.addHook('onSend', function (request, reply, payload, done) { 
        
                 t.pass('onSend called') 
        
                 done() 
        
               }) 
        
               instance.addHook('onResponse', function (request, reply, done) { 
        
                 t.pass('onResponse called') 
        
                 done() 
        
               }) 
        
               done() 
        
             })) 
        
             fastify.get('/', function (req, reply) { 
        
               reply.send({ hello: 'world' }) 
        
             }) 
        
             fastify.inject({ 
        
               method: 'POST', 
        
               url: '/', 
        
               payload: { hello: 'world' } 
        
             }, (err, res) => { 
        
               t.error(err) 
        
               t.equal(res.statusCode, 404) 
        
             }) 
        
           })

We just check that the custom genReqId set by the plugin alters the parent context as we want. Just basically verifying a small use case from a plugin perspective

Uzlopak · 2024-01-14T03:05:35Z

tbh I have no clue if this is now proper implemented. :D

@metcoder95 Please review it properly.

metcoder95

Small comment, but LGTM, nice work! 👍

lib/reqIdGenFactory.js

github-actions · 2025-01-15T00:27:38Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

feat: expose method for setGenReqId on FastifyInstance

62039df

metcoder95 reviewed Jan 9, 2024

View reviewed changes

test/genReqId.test.js Outdated Show resolved Hide resolved

docs/Reference/Server.md Outdated Show resolved Hide resolved

test/genReqId.test.js Show resolved Hide resolved

test/genReqId.test.js Show resolved Hide resolved

Uzlopak reviewed Jan 9, 2024

View reviewed changes

docs/Reference/Server.md Outdated Show resolved Hide resolved

feat: update server.md and add tests

11a42ea

mcollina approved these changes Jan 9, 2024

View reviewed changes

Uzlopak reviewed Jan 10, 2024

View reviewed changes

types/instance.d.ts Outdated Show resolved Hide resolved

Uzlopak approved these changes Jan 10, 2024

View reviewed changes

update comment for setGenReqId in instance.d.ts

3d705df

dancastillo mentioned this pull request Jan 11, 2024

[testing] Expose set gen req id 2 dancastillo/fastify#2

Closed

metcoder95 reviewed Jan 11, 2024

View reviewed changes

fix: add test for fastify-plugin set on root instance

e371249

dancastillo force-pushed the expose-set-gen-req-id branch from 3f5575a to e371249 Compare January 13, 2024 15:09

fix: use fastify-plugin to set on root instance

f26241b

dancastillo requested a review from metcoder95 January 14, 2024 02:36

Merge branch 'main' into expose-set-gen-req-id

e552a2f

metcoder95 reviewed Jan 14, 2024

View reviewed changes

lib/reqIdGenFactory.js Outdated Show resolved Hide resolved

fix: getGenReqId - remove bind

6799e5c

mcollina merged commit 7dc69db into fastify:main Jan 15, 2024

renovate bot mentioned this pull request Apr 3, 2024

chore(deps): update all non-major dependencies jellydn/stepci-demo#88

Merged

1 task

X-oss-byte mentioned this pull request May 29, 2024

[Snyk] Upgrade fastify from 4.19.2 to 4.27.0 X-oss-byte/freeCodeCamp#189

Open

This was referenced May 29, 2024

[Snyk] Upgrade fastify from 4.21.0 to 4.27.0 JamieVangeysel/neo-thermostat-server#101

Closed

[Snyk] Upgrade fastify from 4.21.0 to 4.27.0 JamieVangeysel/neo-thermostat-server#102

Closed

JamieVangeysel mentioned this pull request Jun 5, 2024

[Snyk] Upgrade fastify from 4.21.0 to 4.27.0 JamieVangeysel/neo-thermostat-server#103

Closed

lholmquist mentioned this pull request Jun 5, 2024

[Snyk] Upgrade fastify from 4.21.0 to 4.27.0 nodeshift/faas-js-runtime#353

Closed

JamieVangeysel mentioned this pull request Jun 12, 2024

[Snyk] Upgrade fastify from 4.21.0 to 4.27.0 JamieVangeysel/neo-thermostat-server#107

Closed

densbozz mentioned this pull request Jun 13, 2024

[Snyk] Upgrade fastify from 4.17.0 to 4.27.0 densbozz/aks-store-demo#5

Open

Mykhailo-Sichkaruk mentioned this pull request Jun 14, 2024

[Snyk] Upgrade fastify from 4.10.2 to 4.27.0 Mykhailo-Sichkaruk/TODO-list-API-2.0#21

Closed

JamieVangeysel mentioned this pull request Jun 14, 2024

[Snyk] Upgrade fastify from 4.21.0 to 4.27.0 JamieVangeysel/neo-thermostat-server#108

Closed

Mykhailo-Sichkaruk mentioned this pull request Jun 19, 2024

[Snyk] Upgrade fastify from 4.10.2 to 4.27.0 Mykhailo-Sichkaruk/TODO-list-API-2.0#25

Closed

JamieVangeysel mentioned this pull request Jun 20, 2024

[Snyk] Upgrade fastify from 4.21.0 to 4.27.0 JamieVangeysel/neo-thermostat-server#109

Closed

malhanpraveen mentioned this pull request Jun 21, 2024

[Snyk] Upgrade fastify from 4.9.2 to 4.27.0 eBay/flow-telemetry#9

Open

This was referenced Jun 29, 2024

[Snyk] Upgrade fastify from 4.13.0 to 4.27.0 Summonshr/fastify-test#1

Open

[Snyk] Upgrade fastify from 4.13.0 to 4.27.0 Summonshr/fastify-test#5

Open

github-actions bot locked as resolved and limited conversation to collaborators Jan 15, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: expose method for setGenReqId on FastifyInstance#5259

feat: expose method for setGenReqId on FastifyInstance#5259
mcollina merged 7 commits intofastify:mainfrom
dancastillo:expose-set-gen-req-id

dancastillo commented Jan 9, 2024

Uh oh!

metcoder95 left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mcollina left a comment

Uh oh!

mcollina commented Jan 10, 2024

Uh oh!

Uh oh!

Uzlopak left a comment

Uh oh!

metcoder95 left a comment

Uh oh!

Uzlopak commented Jan 14, 2024

Uh oh!

metcoder95 left a comment

Uh oh!

Uh oh!

github-actions bot commented Jan 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	test('run non-encapsulated plugin hooks on default 404', t => {
	t.plan(6)

	const fastify = Fastify()

	fastify.register(fp(function (instance, options, done) {
	instance.addHook('onRequest', function (req, res, done) {
	t.pass('onRequest called')
	done()
	})

	instance.addHook('preHandler', function (request, reply, done) {
	t.pass('preHandler called')
	done()
	})

	instance.addHook('onSend', function (request, reply, payload, done) {
	t.pass('onSend called')
	done()
	})

	instance.addHook('onResponse', function (request, reply, done) {
	t.pass('onResponse called')
	done()
	})

	done()
	}))

	fastify.get('/', function (req, reply) {
	reply.send({ hello: 'world' })
	})

	fastify.inject({
	method: 'POST',
	url: '/',
	payload: { hello: 'world' }
	}, (err, res) => {
	t.error(err)
	t.equal(res.statusCode, 404)
	})
	})

Uh oh!

Conversation

dancastillo commented Jan 9, 2024

Checklist

Uh oh!

metcoder95 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Jan 10, 2024

Uh oh!

Uh oh!

Uzlopak left a comment

Choose a reason for hiding this comment

Uh oh!

metcoder95 left a comment

Choose a reason for hiding this comment

Uh oh!

Uzlopak commented Jan 14, 2024

Uh oh!

metcoder95 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Jan 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants