-
Notifications
You must be signed in to change notification settings - Fork 518
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(pg): fix instrumentation of ESM-imported pg #1701
Conversation
The ESM imported top-level object is a Module Namespace Object per ECMA262 spec that is, apparently, incompatible with the 'PG' class object that 'pg' returns. The "<module>.defaults" holds the equivalent of `require('pg')`.
Codecov Report
@@ Coverage Diff @@
## main #1701 +/- ##
==========================================
- Coverage 91.67% 91.62% -0.05%
==========================================
Files 139 139
Lines 7145 7151 +6
Branches 1440 1444 +4
==========================================
+ Hits 6550 6552 +2
- Misses 595 599 +4
|
Oh, lint passes. There are a number of warnings from |
I would consider this an acceptable trade-off for testing this. 👍 Edit: created #1731 to come up with a testing strategy for ESM - we'll need this going forward |
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.
Thanks for taking care of this! 🙂
The ESM imported top-level object is a Module Namespace Object per ECMA262 spec that is, apparently, incompatible with the 'PG' class object that 'pg' returns. The ".defaults" holds the equivalent of
require('pg')
.Fixes: #1693
This is a similar fix to #1694. See the discussion there on the use of
module[Symbol.toStringTag] === 'Module'
to sniff out ESM.A dump of
moduleExports
before this change, when loadingpg
as ESM is as follows. This shows how thePG
instance export is replaced with the[Module: null prototype]
and how.default
holds the top-level CommonJS exports.Checklist
npm run test:esm
. The downside of that is that thenpm run test:local
version of this will stop and start a Postgres instance once for CommonJS tests (*.test.js
) and then once again for ESM tests (*.test.mjs
).