Fixes #525: Add eslint-plugin-promise and fix async/Promise issues #527

c3ho · 2019-12-21T00:45:10Z

Issue This PR Addresses

Type of Change

Bugfix: Change which fixes an issue
New Feature: Change which adds functionality
Documentation Update: Change which improves documentation
UI: Change which improves UI

Description

Added eslint rules regarding promises, we have promises that have catches and returns and some don't. These rules ensure consistency

Checklist

Quality: This PR builds and passes our npm test and works locally
Tests: This PR includes thorough tests or an explanation of why it does not
Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not (if applicable)
Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)

c3ho · 2019-12-21T00:46:37Z

Having issues with dealing with inactive-blog-filter.js. It returns a lot of promises.

manekenpix

Just a comment for the misspelled rule. Going to review the errors now.

.eslintrc.js

c3ho · 2019-12-23T15:50:58Z

I think all the functions in storage.js are/should be async no?

c3ho · 2020-01-11T07:16:33Z

Running npm test will fail on the wiki-feed-parser.test

Otherwise it passes every test. Please go easy on me.

manekenpix · 2020-01-11T08:37:24Z

Tests pass for me.

humphd

Looking good, cookie monster. I have done a first read through this, and made some notes for things to fixup or improve. When you've done these, and it's passing CI, we can do another round.

Thanks for doing this! I think it will help people a lot when it's in to avoid making mistakes. You should plan to do a short learning session for the rest of the team to teach them some of the patterns and anti-patterns this revealed as you went.

.eslintrc.js

humphd · 2020-01-11T16:38:02Z

src/backend/index.js

@@ -42,6 +42,7 @@ async function enqueueWikiFeed() {
 enqueueWikiFeed()
  .then(() => {


Often this means you can just rely on the implicit return value you get with an arrow function. I think this is better:

.then(() => feedWorker.start())

humphd · 2020-01-11T16:39:16Z

src/backend/utils/basic_analysis.js

-      }
-    });
+  this.getAsyAnalysis = function() {
+    let result;


Drop this result arg, and just use return on every line where you set result below. Return early, return often.

humphd · 2020-01-11T16:41:53Z

src/backend/utils/email-sender.js

@@ -90,18 +90,16 @@ exports.sendMessage = async function(receipiants, subjectMessage, message) {
    );
    const allGood = this.verifyTransporter(transporter);
    if (!allGood) {
-      reject(new Error()); // Send promise.reject if an error occurs
+      logger.error(new Error());


Get rid of the variable, add a message to the error so it's useful, throw.

if(!this.verifyTransporter(transporter)) { throw new Error('email transport could not be verified'); }

humphd · 2020-01-11T16:43:17Z

src/backend/utils/github-url.js

-    .then(res => res.json())
-    .then(data => {
-      let fetchedData;
+  const fetchResult = await fetch(`${githubAPI}${subUrl}`);


fetch can throw, so can .json(), so we should wrap these in try/catch.

humphd · 2020-01-11T16:46:53Z

src/backend/utils/storage.js

@@ -68,5 +68,8 @@ module.exports = {

  getPostsCount: () => redis.zcard(POSTS),

-  getPost: async guid => redis.hgetall(guid),
+  getPost: async guid => {


No need for async/await here, this already returns a Promise.

getPost: guid => redis.hgetall(guid)

test/email-sender.test.js

humphd · 2020-01-11T16:49:17Z

test/spam-checker.test.js

@@ -1,5 +1,6 @@
 const feedparser = require('feedparser-promised');
 const checkForSpam = require('../src/backend/utils/spam-checker');
+// const { logger } = require('../src/backend/utils/logger');


Remove this if not needed.

humphd · 2020-01-11T16:50:04Z

test/storage.test.js

@@ -81,7 +82,13 @@ describe('Storage tests for posts', () => {
  };

  beforeAll(() => {
-    Promise.all([testPost, testPost2, testPost3].map(post => addPost(post)));
+    Promise.all([testPost, testPost2, testPost3].map(post => addPost(post)))
+      .then(() => {


Why are we mixing async/await and .then? Do we really need this .then at all?

humphd

Getting there. I did another pass over it. It's looking really good, great work.

.eslintrc.js

src/backend/utils/basic_analysis.js

src/backend/utils/github-url.js

humphd · 2020-01-11T23:47:11Z

src/backend/utils/github-url.js

+    }
+    return fetchedData;
+  } catch (error) {
+    throw new Error('Error fetching data');


If all you're going to do in a .catch is throw, there's no value in doing it, and you can just leave it off. However, it might be nice to log the error here, before you throw it. Also, I'd consider just re-throwing the actual error vs. making your own, and losing the original error message and stack.

What you do in src/backend/utils/inactive-blog-filter.js below is what I mean.

src/backend/utils/storage.js

test/inactive-blog-filter.test.js

test/posts.test.js

test/storage.test.js

humphd

A few minor things left, otherwise, this looks great.

humphd · 2020-01-18T00:01:30Z

src/backend/utils/github-url.js


+const log = logger.child({ module: 'github-url' });


Let's not do this. The docs suggest it, but for our needs, it's not really necessary to isolate logs into sub-modules at this stage. Just use logger.info(), etc.

humphd · 2020-01-18T00:04:45Z

src/backend/utils/inactive-blog-filter.js

+  try {
+    const redListRaw = await fsPromises.readFile(redlistPath, 'utf-8');
+    const redList = [].concat(JSON.parse(redListRaw));
+    return redList.some(isRedlisted.bind(null, feedUrl));


Could this not be return redList.some(redItem => isRedlisted(feedUrl, redItem));? It doesn't seem like we need to .bind() here.

.eslintrc.js

humphd · 2020-01-19T20:41:55Z

@c3ho did you ever fix your error above in #527 (comment)? It means you need to rebuild some binary dependency that was linked against header files for a different version of node/v8. If you update node, you sometimes have to rebuild. There's a command to do it, or you can rm -fr node_modules && npm install.

c3ho · 2020-01-19T20:48:15Z

@humphd it is flaky for me on Windows. Sometimes it passes with no issues, sometimes I run into the error. I've done what you've usually suggested in OSD600 removing the dependency folders and everything. I've even tried installing nvm on my windows machine. I tested this on Mac and there were no issues.

humphd

One last set of small fixes, and this is ready, I think. Great work, @c3ho. Let's get this in!

humphd · 2020-01-19T20:43:10Z

src/backend/utils/email-sender.js

+    const result = await transporter.sendMail(mail);
+    return result.accepted;
+  } catch (error) {
+    logger.error(error);


logger.error({ error }, 'Unable to send email');

humphd · 2020-01-19T20:43:52Z

src/backend/utils/github-url.js

+    const fetchResult = await fetch(`${githubAPI}${subUrl}`);
+    data = await fetchResult.json();
+  } catch (error) {
+    logger.error(error);


logger.error({ error }, 'Unable to fetch GitHub API results');

humphd · 2020-01-19T20:44:25Z

src/backend/utils/inactive-blog-filter.js

+  try {
+    const redListRaw = await fsPromises.readFile(redlistPath, 'utf-8');
+    const redList = [].concat(JSON.parse(redListRaw));
+    // return redList.some(isRedlisted.bind(null, feedUrl));


Remove this if it's just commented out.

humphd · 2020-01-19T21:12:01Z

test/posts.test.js

@@ -24,8 +24,8 @@ describe('test /posts endpoint', () => {
    };
  });

-  beforeAll(() => {
-    Promise.all(posts.map(post => addPost(post)));
+  beforeAll(async () => {


Jest's functions work async if you return a Promise, so you can change this to:

beforeAll(() => Promise.all(posts.map(post => addPost(post))));

humphd · 2020-01-19T21:14:15Z

test/posts.test.js

@@ -120,8 +120,8 @@ describe('test /posts/:guid responses', () => {

  // add the post to the storage
  beforeAll(async () => {


beforeAll(() => Promise.all([addedPost1.save(), addedPost2.save()]));

This one got missed.

humphd · 2020-01-19T21:14:57Z

test/storage.test.js

@@ -80,8 +80,8 @@ describe('Storage tests for posts', () => {
    site: 'foo',
  };

-  beforeAll(() => {
-    Promise.all([testPost, testPost2, testPost3].map(post => addPost(post)));
+  beforeAll(async () => {


beforeAll(() => Promise.all([testPost, testPost2, testPost3].map(post => addPost(post))));

humphd

One of my previous comments got missed, after that, this is good to go.

humphd · 2020-01-20T19:54:52Z

test/posts.test.js

@@ -120,8 +120,8 @@ describe('test /posts/:guid responses', () => {

  // add the post to the storage
  beforeAll(async () => {


This one got missed.

c3ho · 2020-01-21T04:35:01Z

@humphd if this is good to go, I'm going to rebase again and resubmit, VSC was going weird on me and added your recent code as part of my PR

humphd

This is failing tests, can you fix and let me know when it's green?

c3ho · 2020-01-21T15:29:53Z

@humphd its failing on this one

Is the fix just a simple await in front of queue.add() (line 33)?

…c/Promise issues

humphd · 2020-01-21T16:12:13Z

I filed #567 to deal with the problem of knowing whether something returns a Promise or not.

humphd

Well done.

manekenpix

👍

humphd · 2020-01-21T20:15:26Z

Yeah, merged!

c3ho force-pushed the issue-525 branch from 79b167d to 5208cf3 Compare December 21, 2019 00:52

manekenpix changed the title ~~Issue 525~~ Fixes #525: Add eslint-plugin-promise and fix async/Promise issues Dec 23, 2019

manekenpix requested changes Dec 23, 2019

View reviewed changes

.eslintrc.js Outdated Show resolved Hide resolved

manekenpix added the developer experience Helping the Developer Experience label Dec 23, 2019

c3ho force-pushed the issue-525 branch from b78ba52 to 04f6e8c Compare January 11, 2020 07:09

manekenpix requested review from Silvyre, cindyorangis, Grommers00, humphd and miggs125 January 11, 2020 08:36

manekenpix self-requested a review January 11, 2020 16:17

humphd requested changes Jan 11, 2020

View reviewed changes

humphd requested changes Jan 18, 2020

View reviewed changes

humphd mentioned this pull request Jan 18, 2020

UnhandledPromiseRejectionWarning in feed-worker.test.js #546

Closed

humphd requested changes Jan 19, 2020

View reviewed changes

c3ho force-pushed the issue-525 branch from 0290de2 to 38b52b6 Compare January 20, 2020 04:56

humphd requested changes Jan 20, 2020

View reviewed changes

c3ho force-pushed the issue-525 branch from 38b52b6 to ee03ded Compare January 21, 2020 04:17

humphd requested changes Jan 21, 2020

View reviewed changes

Fixes issue Seneca-CDOT#525, Added eslint-plugin-promise and fix asyn…

95bb118

…c/Promise issues

c3ho force-pushed the issue-525 branch from 236a869 to 95bb118 Compare January 21, 2020 16:02

humphd approved these changes Jan 21, 2020

View reviewed changes

manekenpix approved these changes Jan 21, 2020

View reviewed changes

c3ho merged commit f185fcf into Seneca-CDOT:master Jan 21, 2020

c3ho deleted the issue-525 branch January 21, 2020 20:14

		@@ -42,6 +42,7 @@ async function enqueueWikiFeed() {
		enqueueWikiFeed()
		.then(() => {

		@@ -120,8 +120,8 @@ describe('test /posts/:guid responses', () => {

		// add the post to the storage
		beforeAll(async () => {

Fixes #525: Add eslint-plugin-promise and fix async/Promise issues #527

Fixes #525: Add eslint-plugin-promise and fix async/Promise issues #527

Conversation

c3ho commented Dec 21, 2019

Issue This PR Addresses

Type of Change

Description

Checklist

c3ho commented Dec 21, 2019

manekenpix left a comment

Choose a reason for hiding this comment

c3ho commented Dec 23, 2019

c3ho commented Jan 11, 2020

manekenpix commented Jan 11, 2020

humphd 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

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

humphd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humphd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humphd commented Jan 19, 2020

c3ho commented Jan 19, 2020

humphd 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

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

humphd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c3ho commented Jan 21, 2020

humphd left a comment

Choose a reason for hiding this comment

c3ho commented Jan 21, 2020

humphd commented Jan 21, 2020

humphd left a comment

Choose a reason for hiding this comment

manekenpix left a comment

Choose a reason for hiding this comment

humphd commented Jan 21, 2020