-
Notifications
You must be signed in to change notification settings - Fork 977
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 support ES modules for Firebase Functions #3485
Conversation
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.
Some nits, but this LGTM
LGTM to me too. This will go into the firebase runtime code before it hits production, but the discovery is still quite useful. |
@@ -567,6 +576,7 @@ async function initializeFirebaseAdminStubs(frb: FunctionsRuntimeBundle): Promis | |||
// Stub the admin module in the require cache | |||
require.cache[adminResolution.resolution] = { | |||
exports: proxiedAdminModule, | |||
path: path.dirname(adminResolution.resolution), |
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.
This needed to be done - otherwise, the dynamic import called failed with errors like this:
TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string. Received undefined
at new NodeError (node:internal/errors:363:5)
at validateString (node:internal/validators:119:11)
at Object.resolve (node:path:1098:7)
at Function.Module._nodeModulePaths (node:internal/modules/cjs/loader:625:17)
at cjsPreparseModuleExports (node:internal/modules/esm/translators:258:30)
at Loader.commonjsStrategy (node:internal/modules/esm/translators:188:35)
@samtstern Think this is safe modification, but I'm 100% unfamiliar with this code path. Let me know if this isn't going to work (I tested things out, and emulator continues to work normally).
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.
This seems safe to me.
\To import modules packaged as ES module, we have to use import instead of require. Both the emulator and function deploy code hardcodes require to load user's function code and ends up throwing ERR_REQUIRE_ESM when given ES modules. We will now try using import when require fails with an ERR_REQUIRE_ESM error. This is a bit lazy - we could've detected whether given module is ES module vs CommonJS and dispatch require vs import appropriately - but I think it simplifies the script while still being correct.
To import modules packaged as ES module, we have to use
import
instead ofrequire
. Both the emulator and function deploy code hardcodesrequire
to load user's function code and ends up throwingERR_REQUIRE_ESM
when given ES modules.We will now try using
import
whenrequire
fails with anERR_REQUIRE_ESM
error. This is a bit lazy - we could've detected whether given module is ES module vs CommonJS and dispatchrequire
vsimport
appropriately - but I think it simplifies the script while still being correct.Note that this PR doesn't itself provide ES module support for Firebase Functions (#2994); we need Google Cloud Function to provide ES module support first (GoogleCloudPlatform/functions-framework-nodejs#233).Functions Framework now supports ES modules in 1.9.0! GoogleCloudPlatform/functions-framework-nodejs#301
NOTE ES Module support is available only for Node.js v13 and beyond. This leads to tricky situations where node version on dev's machine might support ES modules while the selected GCF runtime doesn't, and vice versa. Maybe a follow up item for early detection to reduce number of support tickets we might get because of this confusion?
Will fix once released: #2994