fix: Resolve dual-package hazard for params in ESM #1780
Conversation
We resolve two critical issues caused by the "Dual-Package Hazard" when using `firebase-functions` in ESM projects: 1. Separate instances of the `declaredParams` in `firebase-functions/params` led to params not being detected when used in ESM modules. To fix, we implement a Global Singleton pattern for `declaredParams` using `globalThis` and a version-scoped `Symbol.for` key. This ensures both builds share the same global storage for collecting param API uses. 1. `instanceof` checks failed because the CJS `Expression`/`ResetValue` instances deferred from the ESM ones. To fix, we implement a custom `[Symbol.hasInstance]` on `Expression` and `ResetValue` classes using specfic branded tags to identify instances across package boundaries.
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 provides crucial fixes for the "Dual-Package Hazard" within the Highlights
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
|
![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 effectively addresses the dual-package hazard for parameters in ESM projects by implementing two robust patterns: a global singleton for declaredParams and custom Symbol.hasInstance checks for Expression and ResetValue classes. The use of version-scoped global symbols is a thoughtful touch for ensuring forward compatibility. The changes are well-implemented and include corresponding test cases to validate the fix. I have one minor suggestion to improve the maintainability of the build configuration by reducing some code duplication.
This commit resolves two critical issues caused by the "Dual-Package Hazard" when using `firebase-functions` in ESM projects:
1. **Split State (Missing Params)**:
The SDK bootstrap code (CJS) and User Code (ESM) were using separate instances of the `declaredParams` array.
* **Fix**: Implemented a Global Singleton pattern for `declaredParams` using `globalThis` and a version-scoped `Symbol.for` key. This ensures both builds share the same storage.
2. **Identity Mismatch (Deployment Error)**:
`instanceof Expression` checks in the SDK bootstrap code failed because the user's `Expression` instance (ESM) differed from the bootstrap's class (CJS).
* **Fix**: Implemented `[Symbol.hasInstance]` on `Expression` and `ResetValue` classes.
* **Mechanism**: Uses `Symbol.for` tags (`firebase-functions:Expression:Tag`) to robustly identify instances across package boundaries, avoiding fragile duck-typing or strict class identity checks.
This ensures that parameterized configuration works correctly regardless of the module system used.
…om/firebase/firebase-functions into fix-esm-params-dual-package-hazard
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively resolves the "Dual-Package Hazard" for parameterized configurations in ESM projects. The approach of using a global singleton for declaredParams via globalThis and a version-scoped Symbol.for key is a robust solution to share state between CJS and ESM builds. Similarly, the use of a custom [Symbol.hasInstance] on Expression and ResetValue classes is a clever and correct way to fix instanceof checks across different module instances. The changes are well-implemented and include new tests to cover the fixes. Overall, this is a high-quality contribution that addresses a tricky problem. I have one minor suggestion regarding code style.
jhuleatt
left a comment
jhuleatt
left a comment
There was a problem hiding this comment.
gnarly!
Once Node 20 is EOL in the spring, we should be able to stop publishing the cjs package entirely: https://nodejs.org/api/modules.html#loading-ecmascript-modules-using-require. Hopefully that will simplify things!
* fix: Resolve dual-package hazard for params in ESM
We resolve two critical issues caused by the "Dual-Package Hazard" when using `firebase-functions` in ESM projects:
1. Separate instances of the `declaredParams` in `firebase-functions/params` led to params not being detected when used in ESM modules.
To fix, we implement a Global Singleton pattern for `declaredParams` using `globalThis` and a version-scoped `Symbol.for` key. This ensures both builds share the same global storage for collecting param API uses.
1. `instanceof` checks failed because the CJS `Expression`/`ResetValue` instances deferred from the ESM ones.
To fix, we implement a custom `[Symbol.hasInstance]` on `Expression` and `ResetValue` classes using specfic branded tags to identify instances across package boundaries.
* fix: Resolve dual-package hazard for params in ESM
This commit resolves two critical issues caused by the "Dual-Package Hazard" when using `firebase-functions` in ESM projects:
1. **Split State (Missing Params)**:
The SDK bootstrap code (CJS) and User Code (ESM) were using separate instances of the `declaredParams` array.
* **Fix**: Implemented a Global Singleton pattern for `declaredParams` using `globalThis` and a version-scoped `Symbol.for` key. This ensures both builds share the same storage.
2. **Identity Mismatch (Deployment Error)**:
`instanceof Expression` checks in the SDK bootstrap code failed because the user's `Expression` instance (ESM) differed from the bootstrap's class (CJS).
* **Fix**: Implemented `[Symbol.hasInstance]` on `Expression` and `ResetValue` classes.
* **Mechanism**: Uses `Symbol.for` tags (`firebase-functions:Expression:Tag`) to robustly identify instances across package boundaries, avoiding fragile duck-typing or strict class identity checks.
This ensures that parameterized configuration works correctly regardless of the module system used.
* fix changelog
* clarify comments.
* formatter
2 participants
This PR resolves two critical issues caused by the "Dual-Package Hazard" when using
firebase-functionsin ESM projects:declaredParamsinfirebase-functions/paramsfor CJS and ESM led to params not being detected when used in ESM modules.To fix, we implement a global singleton pattern for
declaredParamsusingglobalThisand a version-scopedSymbol.forkey. This ensures both builds share the same global storage for collecting param API uses.instanceofchecks failed because the CJSExpression/ResetValueinstances differed from the ESM instances.To fix, we implement a custom
[Symbol.hasInstance]onExpressionandResetValueclasses using branded tags to identify instances across package boundaries.Fixes #1775, #1777, #1779