-
Notifications
You must be signed in to change notification settings - Fork 513
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
fix(fastify): Make sure consturctor patching works with esm #1624
Conversation
Is there an impact to commonjs users using fastify like this (copied from fastify docs)? const fastify = require('fastify')({
logger: true
}) |
I've tried it in a small test app of mine where both syntaxes continue to work for CJS, but would appreciate if somebody else could also give it a go (due to the "mysterious" ways that these things sometimes work 😅 ) |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1624 +/- ##
=======================================
Coverage 91.77% 91.77%
=======================================
Files 139 139
Lines 7112 7112
Branches 1427 1427
=======================================
Hits 6527 6527
Misses 585 585
|
@mydea thank you for your contribution. Would you mind signing the CLA? Then we can proceed with the PR. 🙂 |
Align this with how other instrumentations wrap.
Done! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an impact to commonjs users using fastify like this (copied from fastify docs)?
const fastify = require('fastify')({ logger: true })I've tried it in a small test app of mine where both syntaxes continue to work for CJS, but would appreciate if somebody else could also give it a go (due to the "mysterious" ways that these things sometimes work 😅 )
Looks good, thanks! 🙂 I also checked and it seems to work for me too.
It seems to need some adjustments so that prettier
succeeds, though (see here, you can also run npm run lint
from the plugins/node/opentelemetry-instrumentation-fastify/
to check locally), then this should be good to go 🙂
ah, right, I ran |
Which problem is this PR solving?
instrumentation-fastify does not work in ESM mode. This PR fixes this so that fastify can be instrumented when using esm.
The underlying problem is that in ESM mode, the
moduleExports
is not === the fastify constructor. Instead, this can only be found under thedefault
key.Short description of the changes
This changes the wrapping behavior to always use the named
fastify
export, which should always exist.Fixes #1519