From c2f6b9aa0906fdde30446d7d20ae5e9a45e97032 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Tue, 25 Jul 2017 12:32:15 +1000 Subject: [PATCH] Suggestions for refactoring --- graph/context.js | 2 +- graph/subscriptions.js | 2 +- services/kue.js | 4 ++-- services/mailer.js | 9 +++++---- services/passport.js | 16 +++------------- services/pubsub.js | 18 ++++++++++-------- services/redis.js | 35 +++++++++++++++++++++++++---------- services/scraper.js | 11 ++++++----- services/users.js | 18 ++++-------------- 9 files changed, 57 insertions(+), 58 deletions(-) diff --git a/graph/context.js b/graph/context.js index 0a94dc30b..cffec0896 100644 --- a/graph/context.js +++ b/graph/context.js @@ -53,7 +53,7 @@ class Context { this.plugins = decorateContextPlugins(this, contextPlugins); // Bind the publish/subscribe to the context. - this.pubsub = pubsub(); + this.pubsub = pubsub.createClient(); } } diff --git a/graph/subscriptions.js b/graph/subscriptions.js index ca505b96d..b8b4f2c0f 100644 --- a/graph/subscriptions.js +++ b/graph/subscriptions.js @@ -133,7 +133,7 @@ const setupFunctions = plugins.get('server', 'setupFunctions').reduce((acc, {plu const createSubscriptionManager = (server) => new SubscriptionServer({ subscriptionManager: new SubscriptionManager({ schema, - pubsub: pubsub(), + pubsub: pubsub.createClient(), setupFunctions, }), onConnect: ({token}, connection) => { diff --git a/services/kue.js b/services/kue.js index b6622d0b7..ffaa817a4 100644 --- a/services/kue.js +++ b/services/kue.js @@ -8,7 +8,7 @@ const kue = module.exports.kue = require('kue'); // Note that unlike what the name createQueue suggests, it currently returns a // singleton Queue instance. So you can configure and use only a single Queue // object within your node.js process. -let Queue = module.exports.queue; +let Queue = module.exports.queue = null; class Task { @@ -22,7 +22,7 @@ class Task { if (!Queue) { module.exports.queue = Queue = kue.createQueue({ redis: { - createClientFactory: () => redis.createClient() + createClientFactory: redis.createClientFactory() } }); } diff --git a/services/mailer.js b/services/mailer.js index 05cc7e88a..2e0e9d1a7 100644 --- a/services/mailer.js +++ b/services/mailer.js @@ -69,6 +69,7 @@ if (SMTP_PORT) { } const defaultTransporter = nodemailer.createTransport(options); +let taskInstance = null; const mailer = module.exports = { @@ -77,15 +78,15 @@ const mailer = module.exports = { */ _task: null, get task() { - if (mailer._task) { - return mailer._task; + if (taskInstance) { + return taskInstance; } - mailer._task = new kue.Task({ + taskInstance = new kue.Task({ name: 'mailer' }); - return mailer._task; + return taskInstance; }, sendSimple({template, locals, to, subject}) { diff --git a/services/passport.js b/services/passport.js index ba83e7c7d..9fc2631b0 100644 --- a/services/passport.js +++ b/services/passport.js @@ -13,17 +13,7 @@ const bowser = require('bowser'); const ms = require('ms'); // Create a redis client to use for authentication. -const {createClient} = require('./redis'); -let _client = null; -const getClient = () => { - if (_client) { - return _client; - } - - _client = createClient(); - - return _client; -}; +const {createClientFactory} = require('./redis'); const { JWT_SECRET, @@ -157,7 +147,7 @@ const HandleLogout = (req, res, next) => { const now = new Date(); const expiry = (jwt.exp - now.getTime() / 1000).toFixed(0); - getClient().set(`jtir[${jwt.jti}]`, now.toISOString(), 'EX', expiry, (err) => { + createClientFactory().set(`jtir[${jwt.jti}]`, now.toISOString(), 'EX', expiry, (err) => { if (err) { return next(err); } @@ -168,7 +158,7 @@ const HandleLogout = (req, res, next) => { }; const checkGeneralTokenBlacklist = (jwt) => new Promise((resolve, reject) => { - getClient().get(`jtir[${jwt.jti}]`, (err, expiry) => { + createClientFactory().get(`jtir[${jwt.jti}]`, (err, expiry) => { if (err) { return reject(err); } diff --git a/services/pubsub.js b/services/pubsub.js index c54097915..56e0f3e4d 100644 --- a/services/pubsub.js +++ b/services/pubsub.js @@ -2,13 +2,15 @@ const {RedisPubSub} = require('graphql-redis-subscriptions'); const {connectionOptions} = require('./redis'); -let pubsub = null; -module.exports = () => { - if (pubsub) { - return pubsub; +let pubsubInstance = null; +module.exports = { + createClient: () => { + if (pubsubInstance) { + return pubsubInstance; + } + + pubsubInstance = new RedisPubSub({connection: connectionOptions}); + + return pubsubInstance; } - - pubsub = new RedisPubSub({connection: connectionOptions}); - - return pubsub; }; diff --git a/services/redis.js b/services/redis.js index c6506eb3c..1983249f1 100644 --- a/services/redis.js +++ b/services/redis.js @@ -29,21 +29,36 @@ const connectionOptions = { } }; +const createClient = () => { + let client = redis.createClient(connectionOptions); + + client.ping((err) => { + if (err) { + console.error('Can\'t ping the redis server!'); + + throw err; + } + + debug('connection established'); + }); + + return client; +}; + module.exports = { connectionOptions, - createClient() { - let client = redis.createClient(connectionOptions); + createClient, + createClientFactory: () => { + let client = null; - client.ping((err) => { - if (err) { - console.error('Can\'t ping the redis server!'); - - throw err; + return () => { + if (client) { + return client; } - debug('connection established'); - }); + client = createClient(); - return client; + return client; + }; } }; diff --git a/services/scraper.js b/services/scraper.js index 2f424185f..66eb32da6 100644 --- a/services/scraper.js +++ b/services/scraper.js @@ -5,6 +5,8 @@ const AssetsService = require('./assets'); const metascraper = require('metascraper'); +let taskInstance = null; + /** * Exposes a service object to allow operations to execute against the scraper. * @type {Object} @@ -14,17 +16,16 @@ const scraper = { /** * Create the new Task kue singleton. */ - _task: null, get task() { - if (scraper._task) { - return scraper._task; + if (taskInstance) { + return taskInstance; } - scraper._task = new kue.Task({ + taskInstance = new kue.Task({ name: 'scraper' }); - return scraper._task; + return taskInstance; }, /** diff --git a/services/users.js b/services/users.js index 7abf62cb4..83a8c3cbe 100644 --- a/services/users.js +++ b/services/users.js @@ -30,17 +30,7 @@ const PASSWORD_RESET_JWT_SUBJECT = 'password_reset'; const SALT_ROUNDS = 10; // Create a redis client to use for authentication. -const {createClient} = require('./redis'); -let _client = null; -const getClient = () => { - if (_client) { - return _client; - } - - _client = createClient(); - - return _client; -}; +const {createClientFactory} = require('./redis'); // UsersService is the interface for the application to interact with the // UserModel through. @@ -77,7 +67,7 @@ module.exports = class UsersService { const rdskey = `la[${email.toLowerCase().trim()}]`; return new Promise((resolve, reject) => { - getClient() + createClientFactory() .multi() .incr(rdskey) .expire(rdskey, RECAPTCHA_WINDOW_SECONDS) @@ -90,7 +80,7 @@ module.exports = class UsersService { if (replies[0] === 1 || replies[1] === -1) { // then expire it after the timeout - getClient().expire(rdskey, RECAPTCHA_WINDOW_SECONDS); + createClientFactory().expire(rdskey, RECAPTCHA_WINDOW_SECONDS); } if (replies[0] >= RECAPTCHA_INCORRECT_TRIGGER) { @@ -112,7 +102,7 @@ module.exports = class UsersService { const rdskey = `la[${email.toLowerCase().trim()}]`; return new Promise((resolve, reject) => { - getClient() + createClientFactory() .get(rdskey, (err, reply) => { if (err) { return reject(err);