Skip to content
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

Follow up to RuntimeWorkers change #1778

Merged
merged 6 commits into from
Nov 8, 2019
Merged

Conversation

samtstern
Copy link
Contributor

@samtstern samtstern commented Nov 8, 2019

Description

This is a follow up to #1733 to make some of the changes that @yuchenshi asked for. This was a nice and productive rabbit hole.

Changes include:

  • Make FunctionsEmulator no longer rely on the options object
  • Change static methods from FunctionsEmulator that were only static for testing purposes
  • Move all of the logging code into EmulatorLogger where it belongs
  • Assign the socketPath for HTTPS functions before the function runs so that we don't have to
    wait async for it to bubble up.
  • Remove the waitForSystemLog function from the worker class, it's not needed anymore.
  • Remove waiting for ready from background functions where it's not needed.

Scenarios Tested

In addition to making the test suite run, I tested that these functions all work as expected:

exports.helloWorld = functions.https.onRequest(async (req, res) => {
    res.json({
        hello: 'world',
        date: new Date()
    });
});

exports.firestoreWriter = functions.https.onRequest(async (req, res) => {
    await admin.firestore().doc('foo/bar').set({
        date: new Date()
    });
});

exports.firestoreReader = functions.firestore.document('foo/bar').onWrite(async (change, ctx) => {
    console.log("Firestore Write:", JSON.stringify(change.after.data()));
});

Sample Commands

N/A

@samtstern samtstern requested a review from yuchenshi November 8, 2019 20:07
@googlebot googlebot added the cla: yes Manual indication that this has passed CLA. label Nov 8, 2019
@@ -69,6 +72,143 @@ export class EmulatorLogger {
}
}

static handleRuntimeLog(log: EmulatorLog, ignore: string[] = []): void {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of this code is just moved over from FunctionsEmulator. No changes.

// For analytics, track the invoked service
track(EVENT_INVOKE, log.data.service);
if (triggerId) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By making this function not static we were able to just used the trigger definitions held on the emu instance rather than waiting for a log.

@@ -293,180 +290,31 @@ export class FunctionsEmulator implements EmulatorInstance {
return worker;
}

static handleSystemLog(systemLog: EmulatorLog): void {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the code that was moved to EmulatorLogger

@coveralls
Copy link

coveralls commented Nov 8, 2019

Coverage Status

Coverage increased (+0.2%) to 66.248% when pulling d5e295b on ss-runtime-workers-follow-up into 52281c0 on master.

src/emulator/functionsRuntimeWorker.ts Outdated Show resolved Hide resolved
src/emulator/functionsRuntimeWorker.ts Show resolved Hide resolved
src/test/emulators/functionsRuntimeWorker.spec.ts Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Manual indication that this has passed CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants