Encapsulate runtime-dependent code #3454

inlined · 2021-06-05T01:03:02Z

Node JS code is now behind /deploy/functions/runtimes/node. The rest of the codebase now only interacts with the runtime through a RuntimeDelegate interface exposed in /deploy/functions/runtimes.

The following files/functions have been renamed:

from	to
`checkFirebaseSDKVersions.ts`	`deploy/functions/runtimes/node/versioning.ts`
`deploy/functions/discovery/jsexports/*`	`deploy/functions/runtimes/node`
`deploy/functions/checkRuntimeDependencies.ts`	`deploy/functions/ensureCloudBuildEnabled.ts`
`deploy/functions/validate.ts`	`deploy/functions/runtimes/node/validate.ts` (Node methods only)

Some code has been refactored to be more coehsive or to remove
redundancies:

All Runtime specific code from backend.ts (and functions API code + pacakge.json parser) has been deduplicated in runtimes/index.ts
Multiple places loaded the current firebase-functions SDK. This is now called by the RuntimeDelegate and passed where it is needed
Context no longer needs anything to do with runtimes. All Runtime-dependent code is handled in prepare
prepareFunctionsUplaod did a lot more than just packaging source. It was previously getting the backend (and storing it in context so it could be reused), getting env vars, getting runtime config, etc. It now just uploads source and everything else is in prepare directly.
The min-SDK version check has been moved from getRuntimeChoice() to checkFunctionsSDKVersion in versioning.ts, which always did other checks. This saved an exec to npm.

Some small fixes/improvements along the way:

The log line we print when a runtime is not specified now tells customers to use the latest nodejs runtime
We warn customers of breaking changes for any breaking change, not just 0.x to 1.x.

inlined · 2021-06-05T01:04:10Z

src/commands/deploy.js

@@ -79,7 +78,6 @@ module.exports = new Command("deploy")
    }
  })
  .before(checkValidTargetFilters)
-  .before(checkFunctionsSDKVersion)


Note: This check was redundant with other functions deploy code. It's now in deploy/functions/runtimes/node/index.ts:Delegate#validate

inlined · 2021-06-05T01:05:12Z

src/deploy/functions/backend.ts

@@ -116,18 +115,6 @@ export function memoryOptionDisplayName(option: MemoryOptions): string {

 export const SCHEDULED_FUNCTION_LABEL = Object.freeze({ deployment: "firebase-schedule" });

-/** Supported runtimes for new Cloud Functions. */
-export type Runtime = "nodejs10" | "nodejs12" | "nodejs14";


de-duplciated in deploy/functions/runtimes

inlined · 2021-06-05T01:12:33Z

src/deploy/functions/runtimes/node/parseRuntimeAndValidateSDK.ts

-  clc.bold("npm i --save firebase-functions@latest") +
-  " in the functions folder.";
-
-function functionsSDKTooOld(sourceDir: string, minRange: string): boolean {


check moved to versioning.ts

inlined · 2021-06-05T01:15:19Z

src/deploy/functions/validate.ts

@@ -49,41 +46,6 @@ export function functionIdsAreValid(functions: { id: string }[]): void {
  }
 }

-/**


moved to deploy/functions/runtimes/node/validate.ts

inlined · 2021-06-05T01:16:49Z

src/test/deploy/functions/runtimes/node/parseRuntimeAndValidateSDK.spec.ts

  let SDKVersionStub: sinon.SinonStub;

  beforeEach(() => {
    cjsonStub = sandbox.stub(cjson, "load");
-    warningSpy = sandbox.spy(utils, "logWarning");


These parts of tests are moved to /test/deploy/functions/runtimes/node/versioning.spec.ts

inlined · 2021-06-05T01:17:23Z

src/test/deploy/functions/runtimes/node/validate.spec.ts

+
+const cjson = require("cjson");
+
+describe("validate", () => {


tests ripped from test/deploy/functions/validate.spec.ts

Node JS code is now behind /deploy/functions/runtimes/node. The rest of the codebase now only interacts with the runtime through a RuntimeDelegate interface exposed in /deploy/functions/runtimes. All Runtime specific code from backend.ts (and functions API code + pacakge.json parser) has been deduplicated in runtimes/index.ts The following files/functions have been renamed: checkFirebaseSDKVersions.ts -> deploy/functions/runtimes/node/versioning.ts deploy/functions/discovery/jsexports/* -> deploy/functions/runtimes/node deploy/functions/checkRuntimeDependencies.ts -> deploy/functions/ensureCloudBuildEnabled.ts deploy/functions/validate.ts -> deploy/functions/runtimes/node/validate.ts (Node methods only) Some code has been refactored to be more coehsive or to remove redundancies: 1. Multiple places loaded the current firebase-functions SDK. This is now called by the RuntimeDelegate and passed where it is needed 2. Context no longer needs anything to do with runtimes. All Runtime-dependent code is handled in prepare 3. prepareFunctionsUplaod did a _lot_ more than just pacakaging source. It was previously getting the backend (and storing it in context so it could be reused), getting env vars, getting runtime config, etc. It now just uploads source and everything else is in prepare direclty. 4. The min-SDK version check has been moved from getRuntimeChoice() to a versioning.ts:checkFunctionsSDKVersion, which always did other checks. This saved an exec to npm. Some small fixes/improvements along the way: 1. The log line we print when a runtime is not specified now tells customers to use the latest nodejs runtime 2. We warn customers of breaking changes for any breaking change, not just 0.x to 1.x.

joehan · 2021-06-07T15:25:29Z

src/deploy/functions/args.ts

@@ -2,6 +2,7 @@ import { ReadStream } from "fs";

 import * as backend from "./backend";
 import * as gcfV2 from "../../gcp/cloudfunctionsv2";
+import * as runtimes from "./runtimes";


joehan

LGTM, with a few test and logging nits.

joehan · 2021-06-07T15:45:34Z

src/deploy/functions/runtimes/node/validate.ts

+): void {
+  const packageJsonFile = path.join(sourceDir, "package.json");
+  if (!fsutils.fileExistsSync(packageJsonFile)) {
+    const msg = `No npm package found in functions source directory. Please run 'npm init' inside ${sourceDirName}`;


Not sure if this error message should recommend running npm init, for 2 reasons:

I think the most common cause of this would be having an incorrect functions.source listed in firebase.json

In general, we should push people toward using firebase init over npm init.
I'd suggest changing this to No npm package found in functions source directory '${sourceDirName}'

joehan · 2021-06-07T15:50:49Z

src/test/deploy/functions/ensureCloudBuildEnabled.ts

-          await expect(checkRuntimeDependencies("test-project", runtime)).to.eventually.be.rejected;
-        });
-      });
+      await expect(ensureCloudBuildEnabled("test-project")).to.eventually.be.rejected;


Nit since you weren't really changing this code: This probably should be calling rejectedWith() and asserting that we throw an error that mentions billing. Same applies for the permissions error test

joehan · 2021-06-07T15:53:03Z

src/test/deploy/functions/runtimes/node/versioning.spec.ts

+  it("Should give an upgrade warning", () => {
+    latestVersion.returns("5.0.1");
+    versioning.checkFunctionsSDKVersion("5.0.0");
+    expect(warningSpy).to.have.been.calledWith(sinon.match("Please upgrade"));


Nit - This test could also assert that the breaking change warning isn't shown

This reverts commit 0052e72.

Node JS code is now behind /deploy/functions/runtimes/node. The rest of the codebase now only interacts with the runtime through a RuntimeDelegate interface exposed in /deploy/functions/runtimes. All Runtime specific code from backend.ts (and functions API code + pacakge.json parser) has been deduplicated in runtimes/index.ts The following files/functions have been renamed: checkFirebaseSDKVersions.ts -> deploy/functions/runtimes/node/versioning.ts deploy/functions/discovery/jsexports/* -> deploy/functions/runtimes/node deploy/functions/checkRuntimeDependencies.ts -> deploy/functions/ensureCloudBuildEnabled.ts deploy/functions/validate.ts -> deploy/functions/runtimes/node/validate.ts (Node methods only) Some code has been refactored to be more coehsive or to remove redundancies: 1. Multiple places loaded the current firebase-functions SDK. This is now called by the RuntimeDelegate and passed where it is needed 2. Context no longer needs anything to do with runtimes. All Runtime-dependent code is handled in prepare 3. prepareFunctionsUplaod did a _lot_ more than just pacakaging source. It was previously getting the backend (and storing it in context so it could be reused), getting env vars, getting runtime config, etc. It now just uploads source and everything else is in prepare direclty. 4. The min-SDK version check has been moved from getRuntimeChoice() to a versioning.ts:checkFunctionsSDKVersion, which always did other checks. This saved an exec to npm. Some small fixes/improvements along the way: 1. The log line we print when a runtime is not specified now tells customers to use the latest nodejs runtime 2. We warn customers of breaking changes for any breaking change, not just 0.x to 1.x.

inlined requested a review from joehan June 5, 2021 01:03

google-cla bot added the cla: yes Manual indication that this has passed CLA. label Jun 5, 2021

inlined commented Jun 5, 2021

View reviewed changes

inlined force-pushed the inlined.language-specs branch from 180a16d to 5a947b3 Compare June 5, 2021 01:25

joehan reviewed Jun 7, 2021

View reviewed changes

joehan approved these changes Jun 7, 2021

View reviewed changes

PR feedback

8f3a8d0

inlined merged commit 0052e72 into master Jun 7, 2021

fredzqm added a commit that referenced this pull request Jun 7, 2021

Revert "Encapsulate runtime-dependent code (#3454)"

54d9344

This reverts commit 0052e72.

fredzqm mentioned this pull request Jun 7, 2021

Add asian-southeast1 to RTDB CLI #3460

Merged

bkendall deleted the inlined.language-specs branch March 18, 2022 23:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Encapsulate runtime-dependent code #3454

Encapsulate runtime-dependent code #3454

inlined commented Jun 5, 2021

inlined Jun 5, 2021

inlined Jun 5, 2021

inlined Jun 5, 2021

inlined Jun 5, 2021

inlined Jun 5, 2021

inlined Jun 5, 2021

joehan Jun 7, 2021

joehan left a comment

joehan Jun 7, 2021

joehan Jun 7, 2021

joehan Jun 7, 2021

                 }
               }
-              /**

Encapsulate runtime-dependent code #3454

Encapsulate runtime-dependent code #3454

Conversation

inlined commented Jun 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joehan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment