Skip to content

Node 6: add Firebase snippets#697

Merged
ace-n merged 3 commits into
masterfrom
fb-snippets
Aug 23, 2018
Merged

Node 6: add Firebase snippets#697
ace-n merged 3 commits into
masterfrom
fb-snippets

Conversation

@ace-n

@ace-n ace-n commented Jul 24, 2018

Copy link
Copy Markdown
Contributor

Do not merge until we figure out the following:

  1. Are these needed within our docs? Yes
  2. Do they work with Node 8? No, they depend on a callback. We'll have to add Node 8 samples separately.
  3. Does the analytics trigger actually work? No, so we'll have to revisit it later.

@ace-n ace-n added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jul 24, 2018
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 24, 2018
@ace-n ace-n requested a review from jmdobry July 24, 2018 18:06
@ace-n

ace-n commented Jul 24, 2018

Copy link
Copy Markdown
Contributor Author

@mogar1980 FYI

@fhinkel fhinkel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, but maybe a firebase person can have a look?

@ace-n

ace-n commented Aug 18, 2018

Copy link
Copy Markdown
Contributor Author

cc @samtstern (Firebase DPE)


// [START functions_firebase_auth]
/**
* Triggered by a change to a Firebase Auth user object.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think we only have triggers for user creation and deletion right now, not on any change to the user object.

Comment thread functions/firebase/index.js Outdated

// [START functions_firebase_analytics]
/**
* Triggered by a Firebase Mobile Analytics log event.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We call it "Google Analytics for Firebase"

@samtstern

Copy link
Copy Markdown

Two small comments but LGTM (assuming they actually work, I did not take the time to deploy them)

@codecov

codecov Bot commented Aug 22, 2018

Copy link
Copy Markdown

Codecov Report

Merging #697 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #697   +/-   ##
=======================================
  Coverage   55.17%   55.17%           
=======================================
  Files           1        1           
  Lines          58       58           
=======================================
  Hits           32       32           
  Misses         26       26

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c9f58b...f328592. Read the comment docs.

@ace-n ace-n changed the title Add firebase snippets Node 6: add Firebase snippets Aug 23, 2018
@ace-n

ace-n commented Aug 23, 2018

Copy link
Copy Markdown
Contributor Author

Removed broken Analytics sample; merging the rest when tests pass.

@ace-n ace-n merged commit 4e4247a into master Aug 23, 2018
@ace-n ace-n deleted the fb-snippets branch August 23, 2018 00:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes This human has signed the Contributor License Agreement. do not merge Indicates a pull request not ready for merge, due to either quality or timing.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants