-
Notifications
You must be signed in to change notification settings - Fork 2k
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 node8 firebase snippets #719
Conversation
Codecov Report
@@ Coverage Diff @@
## master #719 +/- ##
=======================================
Coverage 55.17% 55.17%
=======================================
Files 1 1
Lines 58 58
=======================================
Hits 32 32
Misses 26 26 Continue to review full report at Codecov.
|
functions/node8/index.js
Outdated
|
||
// [START functions_firebase_auth_node8] | ||
/** | ||
* Triggered by a change to a Firebase Auth user object. |
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.
Same comment as before: it's only creation or deletion, not a change.
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.
Done.
const triggerResource = context.resource; | ||
|
||
console.log(`Function triggered by change to: ${triggerResource}`); | ||
console.log(`Admin?: ${!!data.admin}`); |
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.
@stew-r is this correct, or will data.admin
always be undefined
?
TODO: add tests |
Tests have been added - @fhinkel please review? |
console.log(`Function triggered by change to: ${triggerResource}`); | ||
|
||
console.log(`\nOld value:`); | ||
console.log(JSON.stringify(data.oldValue, null, 2)); |
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.
Is the double space necessary? I personally would prefer shorter code without the extra params.
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.
Not strictly necessary, but makes it easier to read. (Without it, everything is compressed into one line.)
functions/node8/package.json
Outdated
@@ -19,7 +19,9 @@ | |||
"url": "https://github.com/GoogleCloudPlatform/nodejs-docs-samples/issues" | |||
}, | |||
"dependencies": { | |||
"ava": "^0.25.0" | |||
"@google-cloud/nodejs-repo-tools": "^2.3.3", |
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.
repo-tools
and ava
should be dev dependencies.
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.
Done.
functions/node8/test/index.test.js
Outdated
'use strict'; | ||
|
||
const uuid = require('uuid'); | ||
const test = require(`ava`); |
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.
Inconsistent quotes.
* Add node8 firebase snippets * Address sam's comments * Add (unit) tests for Firebase snippets * Move deps to devDeps * Fix quotes
cc @stew-r @samtstern