feat: enable ESM support via dual-publishing #1750
Conversation
Summary of ChangesHello @taeold, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly modernizes the project's module system by introducing dual-publishing support for CommonJS and ECMAScript Modules. This change enables greater flexibility for developers consuming the package, allowing them to integrate it seamlessly into projects using either module standard. The implementation involved comprehensive updates to the package.json's export maps, a shift in the build toolchain to tsdown for generating both CJS and ESM outputs, and meticulous adjustments to type exports and proto generation scripts to ensure full compatibility and type hygiene. A new integration test has also been added to verify the integrity of the packaging. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
taeold
force-pushed
the
esm-js-extensions
branch
from
160680b to
71fa2e6
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces dual CJS/ESM support, which is a significant and well-executed change. The package.json is correctly configured with the exports map, and tsdown is used to generate both module formats. The use of export type improves type hygiene, which is crucial for dual-publishing. The new packaging integration test is a great addition to ensure the package works correctly for both require and import. I have a couple of suggestions to slightly improve the shell script for updating protobufs by reducing some duplication.
taeold
force-pushed
the
esm-js-extensions
branch
9 times, most recently
from
25f832d to
a3c7872
Compare
- Enable esModuleInterop in tsconfig.release.json for better CJS/ESM interop. - Update unit tests and source code to use default imports for CJS modules (fs, nock, jsonwebtoken, etc.) to align with new compiler settings.
taeold
force-pushed
the
esm-js-extensions
branch
from
a3c7872 to
45c5166
Compare
- Enable esModuleInterop in tsconfig.release.json for better CJS/ESM interop. - Update unit tests and source code to use default imports for CJS modules (fs, nock, jsonwebtoken, express, etc.) to align with new compiler settings.
- Configure tsdown to build both CJS and ESM outputs. - Update package.json exports to support both 'require' and 'import'. - Fix type hygiene issues by using explicit 'export type'. - Update protos/update.sh to generate ESM version of compiledFirestore. - Configure build aliases to handle relative paths to protos correctly.
taeold
force-pushed
the
esm-js-extensions
branch
from
45c5166 to
4fc93a4
Compare
- Enable esModuleInterop in tsconfig.release.json for better CJS/ESM interop. - Update unit tests and source code to use default imports for CJS modules (fs, nock, jsonwebtoken, etc.) to align with new compiler settings.
- Enable esModuleInterop in tsconfig.release.json for better CJS/ESM interop. - Update unit tests and source code to use default imports for CJS modules (fs, nock, jsonwebtoken, express, etc.) to align with new compiler settings.
- Configure tsdown to build both CJS and ESM outputs. - Update package.json exports to support both 'require' and 'import'. - Fix type hygiene issues by using explicit 'export type'. - Update protos/update.sh to generate ESM version of compiledFirestore. - Configure build aliases to handle relative paths to protos correctly.
…ase-functions into esm-js-extensions
taeold
force-pushed
the
esm-js-extensions
branch
from
69afff4 to
c44a38a
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces ESM support through dual-publishing, a significant and well-executed change. The updates to package.json exports, build scripts using tsdown, and the addition of a packaging integration test are excellent. The type hygiene fixes using export type are also a great touch for a dual-module package. My review includes a couple of suggestions to improve the test scripts for better debuggability and performance.
github-project-automation
bot
moved this to Approved [PR]
in [Cloud] Extensions + Functions
github-project-automation
bot
moved this from Approved [PR]
to Done
in [Cloud] Extensions + Functions
* BREAKING: Upgrade TypeScript to v5.9.3 (#1746) Upgrades from TypeScript v4.3.5 to v5.9.3. All tests pass successfully. * BREAKING: Drop Node.js 14/16 support, require Node.js >=18 (#1747) * BREAKING: Drop Node.js 14/16 support, require Node.js >=18 - Update package.json engines to require Node.js >=18.0.0 - Upgrade @types/node from v14 to v18 - Update TypeScript target from ES2019 to ES2022 - Add useDefineForClassFields: false for backward compatibility - Update CI/CD workflows to test Node.js 18, 20, 22, and 24 - Set default CI/CD Node.js version to 24 - Update integration test documentation * BREAKING: remove functions.config() implementation (#1748) Replace functions.config() implementation with an error that provides clear migration guidance to the params module. This removes the deprecated API as planned for the next major release while helping developers migrate. The error message directs users to: https://firebase.google.com/docs/functions/config-env#migrate-config * chore: migrate to ESLint v9 and Flat Config (#1753) Upgraded eslint to v9 and replaced the legacy .eslintrc.js with the new eslint.config.js format. Applied minor fixes to 4 files (renaming unused catch variables to _e and one small refactor) to satisfy slightly stricter baseline rules from updated plugins. Mostly doing this due to our upgrade in TS to latest v5 version which the old versions of eslint ts plugins no longer support. * chore: enable esModuleInterop (#1751) * feat: add async error handling to onRequest (#1755) BREAKING CHANGE: Unhandled errors in async onRequest handlers now immediately return a 500 Internal Server Error instead of hanging until timeout. This may affect clients relying on 504 Gateway Timeout for retry logic. * BREAKING: Break TS build that uses functions.config() (#1757) Type out `functions.config()` API. Use of `functions.config()` API should now cause a BUILD error at compile time. ```ts import * as functions from "firebase-functions"; import {onRequest} from "firebase-functions/https"; import * as logger from "firebase-functions/logger"; const cfg = functions.config() export const helloWorld = onRequest((request, response) => { logger.info("Hello logs!", cfg); response.send("Hello from Firebase!"); }); ``` ```bash $ npm run build > build > tsc src/index.ts:5:23 - error TS2349: This expression is not callable. Type 'never' has no call signatures. 5 const cfg = functions.config() ~~~~~~ Found 1 error in src/index.ts:5 ``` * Add Firebase Data Connect v2 support (#1727) * Add Firebase Data Connect v2 support * Export dataconnect * Fix export * Rename Extract to VarName, since Extract is a built in type * Include auth context in event payload * Stronger typing for data connect params * Fix formatting and address comments * Apply another suggestion * Add region and fix bug in which event.service, event.connector, event.operation gets populated * Remove locations/... segment * Address comments * Fix linter * feat: enable ESM support via dual-publishing (#1750) - Configure tsdown to build both CJS and ESM outputs. - Update package.json exports to support both 'require' and 'import'. - Fix type hygiene issues by using explicit 'export type'. - Update protos/update.sh to generate ESM version of compiledFirestore and fix its imports for Node.js compatibility. - Configure build aliases to handle relative paths to protos correctly in both CJS and ESM builds. - Add packaging integration test to CI. ~Note: I enabled `esModuleInterop` in tsconfig.release.json to support default imports from CommonJS modules (like cors), which is required for proper ESM interop. This caused some chain-effect that required updating several unit tests and source files to use default imports for other CJS dependencies (e.g., fs, nock, jsonwebtoken) to align with the new compiler configuration.~ Fixed in #1751. * lock down permission per GitHub alert. * update lint rule to enforce prettier rules. * chore: update CHANGELOG for v7.0.0 * format. --------- Co-authored-by: Lisa Jian <ljian997@gmail.com>
2 participants
Note: I enabledFixed in #1751.esModuleInteropin tsconfig.release.json to support default imports from CommonJS modules (like cors), which is required for proper ESM interop. This caused some chain-effect that required updating several unit tests and source files to use default imports for other CJS dependencies (e.g., fs, nock, jsonwebtoken) to align with the new compiler configuration.