Eslintcompliant #306

tinaliang · 2018-01-05T01:01:32Z

Added eslint rules to each functions sample, refactored code to make each sample eslint compliant.

inlined

I started glossing over after I got to ~paypal because a lot of feedback was redundant. This sort of change is exactly the kind of thing that we should have been doing in a launch branch. Then we can

Say that we added tslints and package.json mechanically, approve the method and merge
New CLs would tackle a few (or one) code repo at a time which would give you early feedback and each PR could be load balanced between reviewers.

inlined · 2018-01-05T22:29:04Z

authenticated-json-api/functions/index.js

 */

-'use strict';
+ 'use strict';


Why the spaces on these lines?

This is apparently a known error in Sublime 3. Thanks for catching it!

But it's still present in this file

inlined · 2018-01-05T22:32:36Z

authenticated-json-api/functions/index.js

      // Cache details in the browser for 5 minutes
      res.set('Cache-Control', 'private, max-age=300');
-      res.status(200).json(snapshot.val());
+      return res.status(200).json(snapshot.val());


Really? We need the returns here? Also, as a minor nit, I try to promote a more consistent error handling style:

if (uncommonCase) { return earlyExit; } continueNormalCodeFlow();

So in this case, I'd recommend:

if (snapshot.val() === null) { return res.status(404).json({errorCode: 404, errorMessage: `message '${messageId}' not found`}); } // Cache details in the browser for 5 minutes return res.set('Cache-Control', 'private, max-age=300');

inlined · 2018-01-05T22:33:33Z

child-count/functions/index.js

 * limitations under the License.
 */
-'use strict';
+ 'use strict';


Still confused why our preambles all got an extra space

Spacing still

inlined · 2018-01-05T22:34:01Z

child-count/functions/index.js

    // Return the promise from counterRef.set() so our function 
    // waits for this async event to complete before it exits.
    return collectionRef.once('value')
-        .then(messagesData => counterRef.set(messagesData.numChildren()));


This indentation was correct.

inlined · 2018-01-06T00:29:16Z

child-count/functions/index.js

    else if (!event.data.exists() && event.data.previous.exists()) {
      return (current || 0) - 1;
    }
+    // What to do in other cases?


transaction is a subtle API. Returning null means "delete the data here in a transaction" while returning undefined means "abort the transaction".

OTOH, reading through this code it's inefficient because it calls tranaction() even when nothing might need to be done.

Try the following instead:

let increment; if (event.data.exists() && !event.data.previous.exists()) { increment = 1; } else if (!event.data.exists() && event.data.previous.exists()) { increment = -1; } else { return null; // No need to increment or decrement } return countRef .transaction(current => (current || 0) + increment) .then(() => { return console.log('Counter updated.'); });

Can we take this ☝️ change?

inlined · 2018-01-06T01:13:54Z

image-sharp/functions/index.js

 * limitations under the License.
 */
-'use strict';
+ 'use strict';


inlined · 2018-01-06T01:14:15Z

image-sharp/functions/index.js

  // Create Sharp pipeline for resizing the image and use pipe to read from bucket read stream
  const pipeline = sharp();
  pipeline
-    .resize(THUMB_MAX_WIDTH, THUMB_MAX_HEIGHT)


These spaces were correct.

inlined · 2018-01-06T01:14:25Z

instagram-auth/functions/index.js

 * limitations under the License.
 */
-'use strict';
+ 'use strict';


spaces in this file too

inlined · 2018-01-06T01:17:08Z

line-auth/functions/index.js

+      return rp(getProfileOptions);
+    }
+    // If error other than auth/user-not-found occurred, fail the whole login process
+    throw error;


nit: I prefer return Promise.reject(error); to keep the original stack trace

inlined · 2018-01-06T01:19:15Z

line-auth/functions/index.js

  // STEP 1: Verify with LINE server that a LINE access token is valid
  return rp(verifyTokenOptions)
-    .then(response => {
+  .then(response => {


.thens indent since they are a line continuation. Multi-line thens continue with the closing bracket of the previous statement.

rp(verifyTokenOptions) .then(response => { }).then(userRecord => { }).then(token => { })

inlined

FYI: You can view the changes you made without all the boilerplate additions of ESLint files by visiting 40299c3

inlined · 2018-01-10T19:29:14Z

child-count/functions/index.js

    else if (!event.data.exists() && event.data.previous.exists()) {
      return (current || 0) - 1;
    }
+    // What to do in other cases?


Can we take this ☝️ change?

inlined · 2018-01-10T19:29:25Z

authenticated-json-api/functions/index.js

 */

-'use strict';
+ 'use strict';


But it's still present in this file

inlined · 2018-01-10T19:29:37Z

child-count/functions/index.js

 * limitations under the License.
 */
-'use strict';
+ 'use strict';


Spacing still

inlined · 2018-01-10T19:29:51Z

child-count/functions/index.js

    // Return the promise from counterRef.set() so our function 
    // waits for this async event to complete before it exits.
    return collectionRef.once('value')
-        .then(messagesData => counterRef.set(messagesData.numChildren()));


inlined · 2018-01-10T19:30:07Z

convert-images/functions/index.js

 * limitations under the License.
 */
-'use strict';
+ 'use strict';


Please fix spacing

inlined · 2018-01-10T19:30:27Z

coupon-on-purchase/functions/index.js

 * limitations under the License.
 */
-'use strict';
+ 'use strict';


Please fix spacing in this file

inlined · 2018-01-10T19:30:45Z

delete-unused-accounts-cron/functions/index.js

 * limitations under the License.
 */
-'use strict';
+ 'use strict';


inlined · 2018-01-10T19:31:36Z

delete-unused-accounts-cron/functions/index.js

 * Returns the list of all users with their ID and lastLogin timestamp.
 */
-function getUsers(userIds = [], nextPageToken, accessToken) {
+ function getUsers(userIds = [], nextPageToken, accessToken) {


inlined · 2018-01-10T19:32:04Z

eslint_results.txt

@@ -0,0 +1,113 @@
+=== /Users/tinaliang/functions-samples/assistant-say-number/functions ===


inlined · 2018-01-10T19:32:37Z

exif-images/functions/index.js

-        console.log('Wrote to:', filePath, 'data:', metadata);
-      });
-    });
+    var result = spawn('identify', ['-verbose', tempLocalFile], { capture: [ 'stdout', 'stderr' ]})


nicolasgarnier · 2018-02-06T19:39:14Z

Can you remove the .DS_Store please?

nicolasgarnier

FYI well try to stick to the Google styleguide which uses 4 spaces for line-wrapping: https://google.github.io/styleguide/jsguide.html#formatting-function-expressions

Sometimes automatic formatter fail at indenting this correctly. Also it looks like the auto-formatter is messing with the JSDocs-style comments by removing the heading spaces before the *. I flagged a bunch :)

nicolasgarnier · 2018-02-06T19:52:11Z

child-count/functions/index.js

-  }).then(() => {
-    console.log('Counter updated.');
+    (current || 0) + increment;
+}).then(() => {


fix indentation here (add 2 spaces here and below)

nicolasgarnier · 2018-02-06T19:52:31Z

child-count/functions/index.js

 const functions = require('firebase-functions');
 const admin = require('firebase-admin');
-admin.initializeApp(functions.config().firebase);
+ admin.initializeApp(functions.config().firebase);


Remove leading space

nicolasgarnier · 2018-02-06T19:57:09Z

fcm-notifications/functions/index.js

+        console.error('Failure sending notification to', tokens[index], error);
+        // Cleanup the tokens who are not registered anymore.
+        if (error.code === 'messaging/invalid-registration-token' ||
+          error.code === 'messaging/registration-token-not-registered') {


add 2 spaces indent here.

nicolasgarnier · 2018-02-06T20:00:10Z

ffmpeg-convert-audio/functions/index.js

  return bucket.file(filePath).download({
    destination: tempFilePath
  }).then(() => {
    console.log('Audio downloaded locally to', tempFilePath);


This was fixed upstream (I think).
Might be good to do a re-base here :)

nicolasgarnier · 2018-02-06T20:03:09Z

github-to-slack/functions/index.js

- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
+* Copyright 2016 Google Inc. All Rights Reserved.


Could you add back the heading spaces before the * here?

Same for the other JS Comment in the file

nicolasgarnier · 2018-02-06T20:46:54Z

text-moderation/functions/index.js

- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
+* Copyright 2016 Google Inc. All Rights Reserved.


add heading space before * back

nicolasgarnier · 2018-02-06T20:47:14Z

url-shortener/functions/index.js

- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
+* Copyright 2017 Google Inc. All Rights Reserved.


add heading space before * back

nicolasgarnier · 2018-02-06T20:47:23Z

username-password-auth/functions/index.js

- * See the License for t`he specific language governing permissions and
- * limitations under the License.
- */
+* Copyright 2017 Google Inc. All Rights Reserved.


add heading space before * back

nicolasgarnier · 2018-02-06T20:47:29Z

username-password-auth/functions/index.js

- * If the request method is unsupported (not POST) return a 403 response.
- * If an error occurs log the details and return a 500 response.
- */
+* Authenticate the provided credentials returning a Firebase custom auth token.


add heading space before * back

nicolasgarnier · 2018-02-06T20:47:39Z

username-password-auth/functions/index.js

- * TODO(DEVELOPER): In production you'll need to update this function so that it authenticates with your own credentials system.
- * @returns {Promise<boolean>} success or failure.
- */
+* Authenticate the provided credentials.


add heading space before * back

nicolasgarnier · 2018-02-07T01:10:08Z

Are we adding Linting checks as presubmit too? In that case it seems the changes to package.json and firebase.json file changes are missing no?

nicolasgarnier

Looks like there are some indentation issues now: sometimes zero indentation for line-wrap, sometimes 1 space and I saw some 3 spaces. We should fix all this.

nicolasgarnier · 2018-02-08T00:28:46Z

child-count/functions/index.js

-    else if (!event.data.exists() && event.data.previous.exists()) {
-      return (current || 0) - 1;
-    }
+    (current || 0) + increment;


We need a return here :)

nicolasgarnier · 2018-02-08T00:29:45Z

coupon-on-purchase/functions/index.js

- * Get the Device Tokens for the given user.
- *
- * @param {string} uid The UID of the user.
+   * Get the Device Tokens for the given user.


Too many leading spaces now :P (should be just one)

nicolasgarnier · 2018-02-08T00:31:33Z

delete-old-child-nodes/functions/index.js

-        });
-        // execute all updates in one go and return the result to end the function
-        return ref.update(updates);
+  .onWrite(event => {


This file doesn't need to be changed (the indentation was OK)

nicolasgarnier · 2018-02-08T00:33:17Z

ffmpeg-convert-audio/functions/index.js

-      .audioFrequency(16000)
-      .format('flac')
-      .output(targetTempFilePath);
+    .setFfmpegPath(ffmpeg_static.path)


indentation looks off here.

nicolasgarnier · 2018-02-08T00:33:23Z

ffmpeg-convert-audio/functions/index.js

-      .run();
-   });
- };
+    .on('end', () => {


indentation looks off here.

Would we would want 4 spaces here?

nicolasgarnier · 2018-02-08T00:33:46Z

ffmpeg-convert-audio/functions/index.js

+function reencodeAsync(tempFilePath, targetTempFilePath) {
+  return new Promise((resolve, reject) => {
+    const command = ffmpeg(tempFilePath)
+    .setFfmpegPath(ffmpeg_static.path)


indentation looks off here.

nicolasgarnier · 2018-02-08T00:34:54Z

github-to-slack/functions/index.js

-    },
-    json: true
-  });
+ return rp({


indentation looks off here looks like we went from 2 spaces to 1 space.

nicolasgarnier · 2018-02-08T00:40:40Z

PS: I may have done my review before your latest changes so not sure if they all still apply :)

googlebot · 2018-02-08T00:54:16Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

This reverts commit ad6982d.

inlined and others added 2 commits December 12, 2017 12:06

Add eslint files; still need to add an npm script & fix errors

ad9bb7d

fixed eslint errors first pass

40299c3

tinaliang requested a review from inlined January 5, 2018 01:02

inlined requested changes Jan 6, 2018

View reviewed changes

tinaliang added 2 commits January 8, 2018 15:43

fixed spacing, coding issues up to line-auth function sample

be7da30

fixed spacing errors from isomorphic-react-app function sample onwards

2a53235

tinaliang assigned tinaliang and inlined and unassigned tinaliang Jan 9, 2018

inlined requested changes Jan 10, 2018

View reviewed changes

inlined assigned tinaliang and unassigned inlined Jan 10, 2018

tinaliang added 4 commits February 6, 2018 11:48

remove ds_store

7d4ce94

merged master and fixed merge conflicts

78e8fee

fixed file to be eslint compliant

54ee5eb

removed git_ignore from remote

26131a4

nicolasgarnier reviewed Feb 6, 2018

View reviewed changes

autoformatting js files

6c6f5ba

nicolasgarnier reviewed Feb 8, 2018

View reviewed changes

tinaliang force-pushed the eslintcompliant branch from b408451 to 1daacbe Compare February 8, 2018 02:43

tinaliang added 6 commits February 8, 2018 15:19

fixed username-password-auth/functions/index.js

eec1448

more syntax fixes

d2f0d6d

last round of syntax fixes

6e4d952

fixed comments for template-handlebars/functions/firebaseUser.js

c681d93

fixed indentations

3efe9ca

passes eslint-config-google standards

965e39b

added parans and removed extra function

c157cd4

tinaliang force-pushed the eslintcompliant branch from 68fe5e0 to c157cd4 Compare February 8, 2018 23:25

last round of lint fixes

1d7cf49

tinaliang merged commit ad6982d into master Feb 9, 2018

tinaliang added a commit that referenced this pull request Feb 9, 2018

Revert "Updated function samples to be eslint compliant (#306)"

c6612f4

This reverts commit ad6982d.

nicolasgarnier mentioned this pull request Feb 18, 2018

remove return statement from console.log #327

Merged

nicolasgarnier mentioned this pull request Mar 1, 2018

Add testing, linting, and building #325

Closed

sefk mentioned this pull request Apr 20, 2018

Exif function example, fix "not defined" errors. #378

Merged

kmcnellis deleted the eslintcompliant branch October 1, 2018 18:53

enterstudio mentioned this pull request Dec 4, 2022

[Snyk] Security upgrade stripe from 4.25.0 to 5.1.0 enterstudio/functions-samples#145

Open

KatanMichael mentioned this pull request Dec 4, 2022

[Snyk] Security upgrade stripe from 4.25.0 to 5.1.0 KatanMichael/functions-samples#252

Open

jayserdny mentioned this pull request Dec 5, 2022

[Snyk] Security upgrade stripe from 4.25.0 to 5.1.0 jayserdny/functions-samples#237

Open

		@@ -0,0 +1,113 @@
		=== /Users/tinaliang/functions-samples/assistant-say-number/functions ===

Eslintcompliant #306

Eslintcompliant #306

Uh oh!

Conversation

tinaliang commented Jan 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

inlined left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

inlined left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolasgarnier commented Feb 6, 2018

Uh oh!

nicolasgarnier left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tinaliang commented Jan 5, 2018 •

edited

Loading

tinaliang Feb 8, 2018 •

edited

Loading