-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Add esm exports #3423
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
Add esm exports #3423
Conversation
Still adding more tests, but wanted to open the PR to see how it runs in CI. edit: yay - tests are passing. now I'll add some more. |
Deploying node-postgres with
|
Latest commit: |
8162202
|
Status: | ✅ Deploy successful! |
Preview URL: | https://2ed736ef.node-postgres.pages.dev |
Branch Preview URL: | https://bmc-add-esm-exports.node-postgres.pages.dev |
todo:
|
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.
Pull Request Overview
This PR adds ECMAScript Module (ESM) export wrappers for several pg-related packages to improve compatibility with ESM environments. Key changes include:
- Creation of ESM wrapper files for pg-protocol, pg-pool, pg-native, pg-cursor, and pg-connection-string.
- Addition of test files to verify the ESM exports.
- Update of pg-cursor to use the pg module for retrieving Result and utils.
Reviewed Changes
Copilot reviewed 21 out of 28 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
packages/pg-protocol/esm/index.js | Adds ESM exports for pg-protocol re-exporting from the dist build |
packages/pg-pool/esm/index.mjs | Provides an ESM wrapper for pg-pool with default export |
packages/pg-native/esm/index.mjs | Provides an ESM wrapper for pg-native with default export |
packages/pg-esm-test/pg.test.js | Introduces tests for exported Client, Pool, and default export from pg |
packages/pg-esm-test/pg-query-stream.test.js | Verifies the default export for pg-query-stream |
packages/pg-esm-test/pg-pool.test.js | Validates the Pool constructor export for pg-pool |
packages/pg-esm-test/pg-native.test.js | Validates the Client constructor export for pg-native |
packages/pg-esm-test/pg-cursor.test.js | Verifies the default export for pg-cursor |
packages/pg-esm-test/pg-connection-string.test.js | Confirms the parsing functions export from pg-connection-string |
packages/pg-esm-test/pg-cloudflare.test.js | Tests the CloudflareSocket export, though the test description mismatches |
packages/pg-cursor/index.js | Refactors pg-cursor to import Result and utils from the pg module |
packages/pg-cursor/esm/index.mjs | Provides an ESM wrapper for pg-cursor with default export |
packages/pg-connection-string/esm/index.mjs | Exposes the parse, toClientConfig, and parseIntoClientConfig functions via ESM |
Files not reviewed (7)
- package.json: Language not supported
- packages/pg-cloudflare/package.json: Language not supported
- packages/pg-connection-string/package.json: Language not supported
- packages/pg-cursor/package.json: Language not supported
- packages/pg-esm-test/package.json: Language not supported
- packages/pg-native/package.json: Language not supported
- packages/pg-pool/package.json: Language not supported
Comments suppressed due to low confidence (1)
packages/pg-esm-test/pg-cloudflare.test.js:5
- The test description 'pg-pool' does not match the Cloudflare context; consider renaming it to 'pg-cloudflare' for clarity.
describe('pg-pool', () => {
This feels breaking, since now you are disallowing the imports for import TypeOverrides from 'pg/lib/type-overrides'; |
@B4nan importing from undocumented paths is debatable wether it's breaking or not. To my understanding, in an ESM environment this (your snippet) will get tricky since the source code is mostly CJS, and ESM only has single wrapper index files. |
Fair enough, but how can we do this properly if this was the wrong way? i believe that class is supposed to be used in downstream. |
@B4nan I guess the best way is to make sure that class is also exported as part of the esm-facing index file. |
@B4nan I did what i could by adding a voice: #2369 (comment) |
This builds off of #3407 and hopefully helps address the issue here.