From 2eec87aa80e5f10787c66cef725e6f26c0786953 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Wed, 6 Dec 2017 16:10:42 -0700 Subject: [PATCH 1/8] modified asset loading to be less lazy --- graph/loaders/assets.js | 164 ++++++++++++++++++++++++-------------- graph/loaders/comments.js | 36 ++++----- graph/loaders/settings.js | 16 +++- models/comment.js | 28 +++++-- services/cache.js | 104 ++++++++++++++++++++---- services/hcache.js | 61 -------------- services/settings.js | 6 +- 7 files changed, 245 insertions(+), 170 deletions(-) delete mode 100644 services/hcache.js diff --git a/graph/loaders/assets.js b/graph/loaders/assets.js index 832b7b473..6ea7e6599 100644 --- a/graph/loaders/assets.js +++ b/graph/loaders/assets.js @@ -1,31 +1,14 @@ const DataLoader = require('dataloader'); -const url = require('url'); +const {URL} = require('url'); +const {singleJoinBy, SingletonResolver} = require('./util'); -const errors = require('../../errors'); -const scraper = require('../../services/scraper'); -const util = require('./util'); - -const AssetModel = require('../../models/asset'); -const AssetsService = require('../../services/assets'); - -/** - * Retrieves assets by an array of ids. - * @param {Object} context the context of the request - * @param {Array} ids array of ids to lookup - */ -const genAssetsByID = (context, ids) => AssetModel.find({ +const genAssetsByID = ({connectors: {models: {Asset}}}, ids) => Asset.find({ id: { $in: ids } -}).then(util.singleJoinBy(ids, 'id')); +}).then(singleJoinBy(ids, 'id')); -/** - * [getAssetsByQuery description] - * @param {Object} context the context of the request - * @param {Object} query the query - * @return {Promise} resolves the assets - */ -const getAssetsByQuery = async (context, query) => { +const getAssetsByQuery = async ({connectors: {services: {Assets}}}, query) => { // If we are requesting based on a limit, ask for one more than we want. const limit = query.limit; @@ -33,7 +16,7 @@ const getAssetsByQuery = async (context, query) => { query.limit += 1; } - const nodes = await AssetsService.search(query); + const nodes = await Assets.search(query); // The hasNextPage is always handled the same (ask for one more than we need, // if there is one more, than there is more). @@ -54,58 +37,121 @@ const getAssetsByQuery = async (context, query) => { }; }; -/** - * This endpoint find or creates an asset at the given url when it is loaded. - * @param {Object} context the context of the request - * @param {String} asset_url the url passed in from the query - * @returns {Promise} resolves to the asset - */ -const findOrCreateAssetByURL = async (context, asset_url) => { +const findOrCreateAssetByURL = async (ctx, url) => { - // Verify that the asset_url is parsable. - let parsed_asset_url = url.parse(asset_url); - if (!parsed_asset_url.protocol) { - throw errors.ErrInvalidAssetURL; + // Pull our connectors out of the context. + const { + loaders: { + Assets, + Settings, + }, + connectors: { + models: { + Asset, + }, + services: { + DomainList, + Scraper, + }, + errors: { + ErrInvalidAssetURL, + }, + }, + } = ctx; + + // Try to validate that the url is valid. If the URL constructor throws an + // error, throw our internal ErrInvalidAssetURL instead. This will validate + // that the url contains a valid scheme. + try { + new URL(url); + } catch (err) { + throw ErrInvalidAssetURL; } - let asset = await AssetsService.findOrCreateByUrl(asset_url); + // Try the easy lookup first. + let asset = await Assets.findByUrl(url); + if (asset) { + return asset; + } - // If the asset wasn't scraped before, scrape it! Otherwise just return - // the asset. + // Seems the asset wasn't here yet.. We should do some validation. + + // Check for whitelisting + get the settings at the same time. + const [ + whitelisted, + settings, + ] = await Promise.all([ + DomainList.urlCheck(url), + Settings.load('autoCloseStream closedTimeout'), + ]); + + // If the domain wasn't whitelisted, then we shouldn't create this asset! + if (!whitelisted) { + throw ErrInvalidAssetURL; + } + + // Construct the update operator that we'll use to create the asset. + const update = { + $setOnInsert: { + url, + }, + }; + + // If the auto-close stream is enabled, close the stream after the designated + // timeout. + if (settings.autoCloseStream) { + update.$setOnInsert.closedAt = new Date(Date.now() + settings.closedTimeout * 1000); + } + + // We're using the findOneAndUpdate here instead of a insert to protect + // against race conditions. + asset = await Asset.findOneAndUpdate({ + url, + }, update, { + + // Ensure that if it's new, we return the new object created. + new: true, + + // Perform an upsert in the event that this doesn't exist. + upsert: true, + + // Set the default values if not provided based on the mongoose models. + setDefaultsOnInsert: true, + + // Ensure that we validate the input that we do have. + runValidators: true, + }); + + // If this is a new asset, then we need to scrape it! if (!asset.scraped) { - await scraper.create(asset); + + // Create the Scraper job. + await Scraper.create(asset); } return asset; }; -const findByUrl = async (context, asset_url) => { +const findByUrl = async ({connectors: {errors, services: {Assets}}}, asset_url) => { - // Verify that the asset_url is parsable. - let parsed_asset_url = url.parse(asset_url); - if (!parsed_asset_url.protocol) { + // Try to validate that the url is valid. If the URL constructor throws an + // error, throw our internal ErrInvalidAssetURL instead. This will validate + // that the url contains a valid scheme. + try { + new URL(asset_url); + } catch (err) { throw errors.ErrInvalidAssetURL; } - return AssetsService.findByUrl(asset_url); + return Assets.findByUrl(asset_url); }; -/** - * Creates a set of loaders based on a GraphQL context. - * @param {Object} context the context of the GraphQL request - * @return {Object} object of loaders - */ - -module.exports = (context) => ({ +module.exports = (ctx) => ({ Assets: { - - // TODO: decide whether we want to move these to mutators or not, as in fact - // this operation create a new asset if one isn't found. - getByURL: (url) => findOrCreateAssetByURL(context, url), - - findByUrl: (url) => findByUrl(context, url), - getByQuery: (query) => getAssetsByQuery(context, query), - getByID: new DataLoader((ids) => genAssetsByID(context, ids)), - getAll: new util.SingletonResolver(() => AssetModel.find({})) + getByURL: (url) => findOrCreateAssetByURL(ctx, url), + findByUrl: (url) => findByUrl(ctx, url), + getByQuery: (query) => getAssetsByQuery(ctx, query), + getByID: new DataLoader((ids) => genAssetsByID(ctx, ids)), + getAll: new SingletonResolver(() => ctx.connectors.models.Asset.find({})) } }); diff --git a/graph/loaders/comments.js b/graph/loaders/comments.js index 2ad94deac..050bd6340 100644 --- a/graph/loaders/comments.js +++ b/graph/loaders/comments.js @@ -82,54 +82,52 @@ const getParentCountsByAssetID = (context, asset_ids) => { /** * Retrieves the count of comments based on the passed in query. - * @param {Object} context graph context + * @param {Object} ctx graph context * @param {Object} query query to execute against the comments collection * to compute the counts * @return {Promise} resolves to the counts of the comments from the * query */ -const getCommentCountByQuery = (context, {ids, statuses, asset_id, parent_id, author_id, tags, action_type}) => { - let query = CommentModel.find(); +const getCommentCountByQuery = (ctx, options) => { + const {statuses, asset_id, parent_id, author_id, tags, action_type} = options; // If user queries for statuses other than NONE and/or ACCEPTED statuses, it needs // special privileges. if ( (!statuses || statuses.some((status) => !['NONE', 'ACCEPTED'].includes(status))) && - (context.user == null || !context.user.can(SEARCH_NON_NULL_OR_ACCEPTED_COMMENTS)) + (ctx.user == null || !ctx.user.can(SEARCH_NON_NULL_OR_ACCEPTED_COMMENTS)) ) { return null; } - if (statuses && statuses.length > 0) { - query = query.where({status: {$in: statuses}}); - } - - if (ids) { - query = query.where({id: {$in: ids}}); - } + const query = CommentModel.find(); if (asset_id != null) { - query = query.where({asset_id}); + query.merge({asset_id}); } if (parent_id !== undefined) { - query = query.where({parent_id}); + query.merge({parent_id}); } if (author_id) { - query = query.where({author_id}); + query.merge({author_id}); } - if (context.user != null && context.user.can(SEARCH_OTHERS_COMMENTS) && action_type) { - query = query.where({ + if (ctx.user != null && ctx.user.can(SEARCH_OTHERS_COMMENTS) && action_type) { + query.merge({ [`action_counts.${sc(action_type.toLowerCase())}`]: { $gt: 0, }, }); } - if (tags) { - query = query.find({ + if (statuses && statuses.length > 0) { + query.merge({status: {$in: statuses}}); + } + + if (tags && tags.length > 0) { + query.merge({ 'tags.tag.name': { $in: tags, }, @@ -289,7 +287,7 @@ const getCommentsByQuery = async (ctx, {ids, statuses, asset_id, parent_id, auth let comments = CommentModel.find(); // If user queries for statuses other than NONE and/or ACCEPTED statuses, it needs - // special priviledges. + // special privileges. if ( (!statuses || statuses.some((status) => !['NONE', 'ACCEPTED'].includes(status))) && (ctx.user == null || !ctx.user.can(SEARCH_NON_NULL_OR_ACCEPTED_COMMENTS)) diff --git a/graph/loaders/settings.js b/graph/loaders/settings.js index 2d6189b7d..a7a4b74e1 100644 --- a/graph/loaders/settings.js +++ b/graph/loaders/settings.js @@ -1,11 +1,19 @@ const SettingsService = require('../../services/settings'); -const {SingletonResolver} = require('./util'); +const DataLoader = require('dataloader'); /** * Creates a set of loaders based on a GraphQL context. * @param {Object} context the context of the GraphQL request * @return {Object} object of loaders */ -module.exports = () => ({ - Settings: new SingletonResolver(() => SettingsService.retrieve()) -}); +module.exports = () => { + const loader = new DataLoader((selections) => Promise.all(selections.map((fields) => { + return SettingsService.retrieve(fields); + }))); + + return { + Settings: { + load: (fields = false) => loader.load(fields), + } + }; +}; diff --git a/models/comment.js b/models/comment.js index bc4cecd5d..ae6aa558f 100644 --- a/models/comment.js +++ b/models/comment.js @@ -123,20 +123,23 @@ CommentSchema.index({ background: true, }); -// Add an index that is optimized for sorting based on the action count data. -CommentSchema.index({ - 'created_at': 1, - 'action_counts.flag': 1, -}, { - background: true, -}); - +// Create a sparse index to search across. CommentSchema.index({ 'created_at': 1, 'action_counts.flag': 1, 'status': 1, }, { background: true, + sparse: true, +}); + +// Create a sparse index to search across. +CommentSchema.index({ + 'action_counts.flag': 1, + 'status': 1, +}, { + background: true, + sparse: true, }); // Add an index that is optimized for finding flagged comments. @@ -166,6 +169,15 @@ CommentSchema.index({ background: true, }); +// Optimize for tag searches/counts. +CommentSchema.index({ + 'tags.tag.name': 1, + 'status': 1, +}, { + background: true, + sparse: true, +}); + // Add an index that is optimized for sorting based on the created_at timestamp // but also good at locating comments that have a specific asset id. CommentSchema.index({ diff --git a/services/cache.js b/services/cache.js index 155992fe0..cac05c091 100644 --- a/services/cache.js +++ b/services/cache.js @@ -61,30 +61,36 @@ cache.init = async () => { // This is designed to increment a key and add an expiry iff the key already // exists. - const INCR_SCRIPT = ` - if redis.call('GET', KEYS[1]) ~= false then - redis.call('INCR', KEYS[1]) - redis.call('EXPIRE', KEYS[1], ARGV[1]) - end - `; - cache.client.defineCommand('increx', { numberOfKeys: 1, - lua: INCR_SCRIPT, + lua: ` + if redis.call('GET', KEYS[1]) ~= false then + redis.call('INCR', KEYS[1]) + redis.call('EXPIRE', KEYS[1], ARGV[1]) + end + `, }); // This is designed to decrement a key and add an expiry iff the key already // exists. - const DECR_SCRIPT = ` - if redis.call('GET', KEYS[1]) ~= false then - redis.call('DECR', KEYS[1]) - redis.call('EXPIRE', KEYS[1], ARGV[1]) - end - `; - cache.client.defineCommand('decrex', { numberOfKeys: 1, - lua: DECR_SCRIPT, + lua: ` + if redis.call('GET', KEYS[1]) ~= false then + redis.call('DECR', KEYS[1]) + redis.call('EXPIRE', KEYS[1], ARGV[1]) + end + `, + }); + + cache.client.defineCommand('hincrbyex', { + numberOfKeys: 2, + lua: ` + if redis.call('HGET', KEYS[1], KEYS[2]) ~= false then + redis.call('HINCRBY', KEYS[1], KEYS[2], ARGV[1]) + redis.call('EXPIRE', KEYS[1], ARGV[2]) + end + `, }); }; @@ -270,3 +276,69 @@ cache.set = async (key, value, expiry, kf = keyfunc) => { return cache.client.set(kf(key), reply, 'EX', expiry); }; + +/** + * h is the hash form of the cache. + */ +cache.h = {}; + +cache.h.get = async (key, field = '__default__') => { + + // Get the current value from redis. + const reply = await cache.client.hget(keyfunc(key), field); + + if (typeof reply !== 'undefined' && reply !== null) { + return JSON.parse(reply); + } + + return null; +}; + +cache.h.set = async (key, field = '__default__', value, expiry = 60) => { + + // Serialize the value as JSON. + let reply = JSON.stringify(value); + + return cache.client + .pipeline() + .hset(keyfunc(key), field, reply) + .expire(keyfunc(key), expiry) + .exec(); +}; + +cache.h.invalidate = async (key, field = null) => { + if (field === null) { + return cache.invalidate(key); + } + + debug(`invalidate: ${keyfunc(key)} ${field}`); + + return cache.client.hdel(keyfunc(key), field); +}; + +cache.h.wrap = async (key, field, expiry, work) => { + let value = await cache.h.get(key, field); + if (value !== null) { + debug('wrap: hit', keyfunc(key)); + return value; + } + + debug('wrap: miss', keyfunc(key)); + + value = await work(); + + process.nextTick(async () => { + try { + await cache.h.set(key, field, value, expiry); + debug('wrap: set complete'); + } catch (err) { + console.error(err); + } + }); + + return value; +}; + +cache.h.incr = async (key, field = '__default__', expiry) => cache.client.hincrbyex(keyfunc(key), field, 1, expiry); + +cache.h.decr = async (key, field = '__default__', expiry) => cache.client.hincrbyex(keyfunc(key), field, -1, expiry); diff --git a/services/hcache.js b/services/hcache.js deleted file mode 100644 index 72e52d9eb..000000000 --- a/services/hcache.js +++ /dev/null @@ -1,61 +0,0 @@ -const cache = require('./cache'); -const debug = require('debug')('talk:services:hcache'); - -const kf = (key) => `hcache:${key}`; - -const hcache = module.exports = {}; - -hcache.get = async (key, field = '__default__') => { - - // Get the current value from redis. - const reply = await cache.client.hget(kf(key), field); - - if (typeof reply !== 'undefined' && reply !== null) { - return JSON.parse(reply); - } - - return null; -}; - -hcache.set = async (key, field = '__default__', value, expiry = 60) => { - - // Serialize the value as JSON. - let reply = JSON.stringify(value); - - return cache.client - .pipeline() - .hset(kf(key), field, reply) - .expire(kf(key), expiry) - .exec(); -}; - -hcache.del = async (key, field = null) => { - if (field === null) { - return cache.client.del(kf(key)); - } - - return cache.client.hdel(kf(key), field); -}; - -hcache.wrap = async (key, field, expiry, work) => { - let value = await hcache.get(key, field); - if (value !== null) { - debug('wrap: hit', kf(key)); - return value; - } - - debug('wrap: miss', kf(key)); - - value = await work(); - - process.nextTick(async () => { - try { - await hcache.set(key, field, value, expiry); - debug('wrap: set complete'); - } catch (err) { - console.error(err); - } - }); - - return value; -}; diff --git a/services/settings.js b/services/settings.js index 90ff6bbd6..e8537c9c9 100644 --- a/services/settings.js +++ b/services/settings.js @@ -1,5 +1,5 @@ const SettingModel = require('../models/setting'); -const hcache = require('./hcache'); +const cache = require('./cache'); const errors = require('../errors'); const {dotize} = require('./utils'); @@ -35,7 +35,7 @@ module.exports = class SettingsService { if (process.env.NODE_ENV === 'production') { // When in production, wrap the settings retrieval with a cache. - const settings = await hcache.wrap('settings', fields, 60, () => retrieve(fields)); + const settings = await cache.h.wrap('settings', fields, 60, () => retrieve(fields)); return new SettingModel(settings); } @@ -58,7 +58,7 @@ module.exports = class SettingsService { }); if (process.env.NODE_ENV === 'production') { - await hcache.del('settings'); + await cache.h.invalidate('settings'); } return updatedSettings; From 7072aa013bed716e967f4ee5fa5cc2e751e62ea2 Mon Sep 17 00:00:00 2001 From: Chi Vinh Le Date: Thu, 7 Dec 2017 18:41:46 +0100 Subject: [PATCH 2/8] document optimizations v2 --- client/coral-framework/graphql/anywhere.js | 23 ++++++++++++++++++++ client/coral-framework/services/bootstrap.js | 4 ---- client/coral-framework/services/graphql.js | 18 +-------------- package.json | 2 +- webpack.config.js | 2 +- yarn.lock | 12 +++++----- 6 files changed, 32 insertions(+), 29 deletions(-) create mode 100644 client/coral-framework/graphql/anywhere.js diff --git a/client/coral-framework/graphql/anywhere.js b/client/coral-framework/graphql/anywhere.js new file mode 100644 index 000000000..5e8496a98 --- /dev/null +++ b/client/coral-framework/graphql/anywhere.js @@ -0,0 +1,23 @@ +import graphql from '@coralproject/graphql-anywhere-optimized'; +import {createTypeGetter} from 'graphql-ast-tools'; +import introspectionData from './introspection.json'; + +// User typeGetter to get more optimized documents. +const typeGetter = createTypeGetter(introspectionData); + +// Use global fragment cache for transformed fragments. +const fragmentMap = {}; + +export default (...args) => { + while (args.length < 7) { + args.push(undefined); + } + + const transformOptions = { + typeGetter, + fragmentMap, + }; + + args[6] = transformOptions; + return graphql(...args); +}; diff --git a/client/coral-framework/services/bootstrap.js b/client/coral-framework/services/bootstrap.js index 3ef6629c1..1328b169d 100644 --- a/client/coral-framework/services/bootstrap.js +++ b/client/coral-framework/services/bootstrap.js @@ -124,10 +124,6 @@ export async function createContext({ const plugins = createPluginsService(pluginsConfig); const graphql = createGraphQLService( createGraphQLRegistry(plugins.getSlotFragments.bind(plugins)), - { - introspectionData, - optimize: process.env.NODE_ENV === 'production', - }, ); if (!notification) { diff --git a/client/coral-framework/services/graphql.js b/client/coral-framework/services/graphql.js index fb40ecf71..d7b132bd4 100644 --- a/client/coral-framework/services/graphql.js +++ b/client/coral-framework/services/graphql.js @@ -1,4 +1,3 @@ -import {transformDocument, createTypeGetter} from 'graphql-ast-tools'; import {addTypenameToDocument} from 'apollo-client/queries/queryTransform'; /** @@ -6,18 +5,7 @@ import {addTypenameToDocument} from 'apollo-client/queries/queryTransform'; * @param {string} basename base path of the url * @return {Object} histor service */ -export function createGraphQLService(registry, { - introspectionData, - optimize = false, -}) { - const transformOptions = { - typeGetter: optimize && introspectionData ? createTypeGetter(introspectionData) : null, - - // Use shared fragment map. - // Attention: Fragment names must be unique otherwise weird things will happen. - fragmentMap: {}, - }; - +export function createGraphQLService(registry) { return { registry, resolveDocument(documentOrCallback, props, context) { @@ -26,10 +14,6 @@ export function createGraphQLService(registry, { : documentOrCallback; document = registry.resolveFragments(document); - if (optimize) { - document = transformDocument(document, transformOptions); - } - // We also add typenames to the document which apollo would usually do, // but we also use the network interface in subscriptions directly // which require the resolved typenames. diff --git a/package.json b/package.json index 4712a75c4..2a4c8fa99 100644 --- a/package.json +++ b/package.json @@ -106,7 +106,7 @@ "fs-extra": "^4.0.1", "gql-merge": "^0.0.4", "graphql": "^0.9.1", - "graphql-ast-tools": "^0.1.5", + "graphql-ast-tools": "0.2.2", "graphql-docs": "0.2.0", "graphql-errors": "^2.1.0", "graphql-redis-subscriptions": "1.3.0", diff --git a/webpack.config.js b/webpack.config.js index 60a49b6eb..14ac7caf5 100644 --- a/webpack.config.js +++ b/webpack.config.js @@ -120,7 +120,7 @@ const config = { }, resolve: { alias: { - 'graphql-anywhere': '@coralproject/graphql-anywhere-optimized', + 'graphql-anywhere': path.resolve(__dirname, 'client/coral-framework/graphql/anywhere'), 'plugin-api': path.resolve(__dirname, 'plugin-api/'), plugins: path.resolve(__dirname, 'plugins/'), pluginsConfig: pluginsPath diff --git a/yarn.lock b/yarn.lock index c01e2e85a..55439436c 100644 --- a/yarn.lock +++ b/yarn.lock @@ -12,10 +12,10 @@ eslint-plugin-react "^7.3.0" "@coralproject/graphql-anywhere-optimized@^0.1.0": - version "0.1.3" - resolved "https://registry.yarnpkg.com/@coralproject/graphql-anywhere-optimized/-/graphql-anywhere-optimized-0.1.3.tgz#f92f3906bb04f001aef725697237786752c49bd6" + version "0.1.5" + resolved "https://registry.yarnpkg.com/@coralproject/graphql-anywhere-optimized/-/graphql-anywhere-optimized-0.1.5.tgz#67c862bf908ea717d9521ea76266b5bc9f109c65" dependencies: - graphql-ast-tools "^0.1.6" + graphql-ast-tools "^0.2.2" "@kadira/storybook-deployer@^1.1.0": version "1.2.0" @@ -3521,9 +3521,9 @@ graphql-anywhere@^3.0.1: version "3.1.0" resolved "https://registry.yarnpkg.com/graphql-anywhere/-/graphql-anywhere-3.1.0.tgz#3ea0d8e8646b5cee68035016a9a7557c15c21e96" -graphql-ast-tools@^0.1.5, graphql-ast-tools@^0.1.6: - version "0.1.6" - resolved "https://registry.yarnpkg.com/graphql-ast-tools/-/graphql-ast-tools-0.1.6.tgz#48eb656434bf4c7dba2a0d4784f1fdb988ab70ed" +graphql-ast-tools@0.2.2, graphql-ast-tools@^0.2.2: + version "0.2.2" + resolved "https://registry.yarnpkg.com/graphql-ast-tools/-/graphql-ast-tools-0.2.2.tgz#2f149b427697a5d966825e1a9b4be0d7a0608bd6" dependencies: apollo-utilities "^1.0.3" From 3ac301142e531e76c009e03bc1b6b5c8ef1b3358 Mon Sep 17 00:00:00 2001 From: Chi Vinh Le Date: Thu, 7 Dec 2017 19:43:32 +0100 Subject: [PATCH 3/8] Update graphql-ast-tools --- package.json | 2 +- yarn.lock | 8 +++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/package.json b/package.json index 2a4c8fa99..7a8766a1e 100644 --- a/package.json +++ b/package.json @@ -106,7 +106,7 @@ "fs-extra": "^4.0.1", "gql-merge": "^0.0.4", "graphql": "^0.9.1", - "graphql-ast-tools": "0.2.2", + "graphql-ast-tools": "0.2.3", "graphql-docs": "0.2.0", "graphql-errors": "^2.1.0", "graphql-redis-subscriptions": "1.3.0", diff --git a/yarn.lock b/yarn.lock index 55439436c..a6ca84229 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3521,7 +3521,13 @@ graphql-anywhere@^3.0.1: version "3.1.0" resolved "https://registry.yarnpkg.com/graphql-anywhere/-/graphql-anywhere-3.1.0.tgz#3ea0d8e8646b5cee68035016a9a7557c15c21e96" -graphql-ast-tools@0.2.2, graphql-ast-tools@^0.2.2: +graphql-ast-tools@0.2.3: + version "0.2.3" + resolved "https://registry.yarnpkg.com/graphql-ast-tools/-/graphql-ast-tools-0.2.3.tgz#447fb05905ebb90f0a5bba81d5715249e9937135" + dependencies: + apollo-utilities "^1.0.3" + +graphql-ast-tools@^0.2.2: version "0.2.2" resolved "https://registry.yarnpkg.com/graphql-ast-tools/-/graphql-ast-tools-0.2.2.tgz#2f149b427697a5d966825e1a9b4be0d7a0608bd6" dependencies: From 07ddb94327b24eb98ff1bec54cc9a8353955981f Mon Sep 17 00:00:00 2001 From: Chi Vinh Le Date: Thu, 7 Dec 2017 20:23:05 +0100 Subject: [PATCH 4/8] Fix yarn.lock --- yarn.lock | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/yarn.lock b/yarn.lock index a6ca84229..ea99f0cfa 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3521,18 +3521,12 @@ graphql-anywhere@^3.0.1: version "3.1.0" resolved "https://registry.yarnpkg.com/graphql-anywhere/-/graphql-anywhere-3.1.0.tgz#3ea0d8e8646b5cee68035016a9a7557c15c21e96" -graphql-ast-tools@0.2.3: +graphql-ast-tools@0.2.3, graphql-ast-tools@^0.2.2: version "0.2.3" resolved "https://registry.yarnpkg.com/graphql-ast-tools/-/graphql-ast-tools-0.2.3.tgz#447fb05905ebb90f0a5bba81d5715249e9937135" dependencies: apollo-utilities "^1.0.3" -graphql-ast-tools@^0.2.2: - version "0.2.2" - resolved "https://registry.yarnpkg.com/graphql-ast-tools/-/graphql-ast-tools-0.2.2.tgz#2f149b427697a5d966825e1a9b4be0d7a0608bd6" - dependencies: - apollo-utilities "^1.0.3" - graphql-docs@0.2.0: version "0.2.0" resolved "https://registry.yarnpkg.com/graphql-docs/-/graphql-docs-0.2.0.tgz#cf803f9c9d354fa03e89073d74e419261a5bfa74" From ba572fbf4846edaf07edf6ffc2402e9f79e32c1f Mon Sep 17 00:00:00 2001 From: Chi Vinh Le Date: Thu, 7 Dec 2017 20:24:45 +0100 Subject: [PATCH 5/8] Typo --- client/coral-framework/graphql/anywhere.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/coral-framework/graphql/anywhere.js b/client/coral-framework/graphql/anywhere.js index 5e8496a98..7a93f16ea 100644 --- a/client/coral-framework/graphql/anywhere.js +++ b/client/coral-framework/graphql/anywhere.js @@ -2,7 +2,7 @@ import graphql from '@coralproject/graphql-anywhere-optimized'; import {createTypeGetter} from 'graphql-ast-tools'; import introspectionData from './introspection.json'; -// User typeGetter to get more optimized documents. +// Use typeGetter to get more optimized documents. const typeGetter = createTypeGetter(introspectionData); // Use global fragment cache for transformed fragments. From d0d047f6414f06b4d1183c1b7e58c71631d59413 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Fri, 8 Dec 2017 15:04:33 -0700 Subject: [PATCH 6/8] fixed bug with load more --- client/coral-embed-stream/src/actions/auth.js | 8 ++++++-- client/coral-embed-stream/src/graphql/utils.js | 10 +++++----- .../src/tabs/stream/containers/Stream.js | 2 +- 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/client/coral-embed-stream/src/actions/auth.js b/client/coral-embed-stream/src/actions/auth.js index 22d46a768..4599a1cc7 100644 --- a/client/coral-embed-stream/src/actions/auth.js +++ b/client/coral-embed-stream/src/actions/auth.js @@ -304,6 +304,8 @@ const checkLoginSuccess = (user, isAdmin) => ({ isAdmin }); +const ErrNotLoggedIn = new Error('Not logged in'); + export const checkLogin = () => (dispatch, _, {rest, client, pym, storage}) => { dispatch(checkLoginRequest()); rest('/auth') @@ -312,7 +314,7 @@ export const checkLogin = () => (dispatch, _, {rest, client, pym, storage}) => { if (storage) { storage.removeItem('token'); } - throw new Error('Not logged in'); + throw ErrNotLoggedIn; } // Reset the websocket. @@ -327,7 +329,9 @@ export const checkLogin = () => (dispatch, _, {rest, client, pym, storage}) => { } }) .catch((error) => { - console.error(error); + if (error !== ErrNotLoggedIn) { + console.error(error); + } if (error.status && error.status === 401 && storage) { // Unauthorized. diff --git a/client/coral-embed-stream/src/graphql/utils.js b/client/coral-embed-stream/src/graphql/utils.js index 0c46b45e3..8ab24d91f 100644 --- a/client/coral-embed-stream/src/graphql/utils.js +++ b/client/coral-embed-stream/src/graphql/utils.js @@ -141,18 +141,18 @@ export function findCommentWithId(nodes, id) { return findComment(nodes, (node) => node.id === id); } -export function findCommentInEmbedQuery(root, callbackOrId) { +export function findCommentInEmbedQuery(props, callbackOrId) { let callback = callbackOrId; if (typeof callbackOrId === 'string') { callback = (node) => node.id === callbackOrId; } - if (root.asset.comment) { - return findComment([getTopLevelParent(root.asset.comment)], callback); + if (props.asset.comment) { + return findComment([getTopLevelParent(props.asset.comment)], callback); } - if (!root.asset.comments) { + if (!props.asset.comments) { return false; } - return findComment(root.asset.comments.nodes, callback); + return findComment(props.asset.comments.nodes, callback); } function findAndInsertFetchedComments(parent, comments, parent_id) { diff --git a/client/coral-embed-stream/src/tabs/stream/containers/Stream.js b/client/coral-embed-stream/src/tabs/stream/containers/Stream.js index dad69b960..b4f375632 100644 --- a/client/coral-embed-stream/src/tabs/stream/containers/Stream.js +++ b/client/coral-embed-stream/src/tabs/stream/containers/Stream.js @@ -108,7 +108,7 @@ class StreamContainer extends React.Component { } loadNewReplies = (parent_id) => { - const comment = findCommentInEmbedQuery(this.props.root, parent_id); + const comment = findCommentInEmbedQuery(this.props, parent_id); return this.props.data.fetchMore({ query: LOAD_MORE_QUERY, From 485b9d036f9dde0d4bc723a9c8efb68b6b708445 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Fri, 8 Dec 2017 15:07:15 -0700 Subject: [PATCH 7/8] version bump --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 7a8766a1e..1273015fe 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "talk", - "version": "3.8.1", + "version": "3.8.2", "description": "A better commenting experience from Mozilla, The New York Times, and the Washington Post. https://coralproject.net", "main": "app.js", "private": true, From 61d16fbe08902162211de8b1163ca3fb110e98f9 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Fri, 8 Dec 2017 15:17:28 -0700 Subject: [PATCH 8/8] Revert "Optimizations v2" --- client/coral-embed-stream/src/actions/auth.js | 8 ++----- .../coral-embed-stream/src/graphql/utils.js | 10 ++++---- .../src/tabs/stream/containers/Stream.js | 2 +- client/coral-framework/graphql/anywhere.js | 23 ------------------- client/coral-framework/services/bootstrap.js | 4 ++++ client/coral-framework/services/graphql.js | 18 ++++++++++++++- package.json | 4 ++-- webpack.config.js | 2 +- yarn.lock | 12 +++++----- 9 files changed, 38 insertions(+), 45 deletions(-) delete mode 100644 client/coral-framework/graphql/anywhere.js diff --git a/client/coral-embed-stream/src/actions/auth.js b/client/coral-embed-stream/src/actions/auth.js index 4599a1cc7..22d46a768 100644 --- a/client/coral-embed-stream/src/actions/auth.js +++ b/client/coral-embed-stream/src/actions/auth.js @@ -304,8 +304,6 @@ const checkLoginSuccess = (user, isAdmin) => ({ isAdmin }); -const ErrNotLoggedIn = new Error('Not logged in'); - export const checkLogin = () => (dispatch, _, {rest, client, pym, storage}) => { dispatch(checkLoginRequest()); rest('/auth') @@ -314,7 +312,7 @@ export const checkLogin = () => (dispatch, _, {rest, client, pym, storage}) => { if (storage) { storage.removeItem('token'); } - throw ErrNotLoggedIn; + throw new Error('Not logged in'); } // Reset the websocket. @@ -329,9 +327,7 @@ export const checkLogin = () => (dispatch, _, {rest, client, pym, storage}) => { } }) .catch((error) => { - if (error !== ErrNotLoggedIn) { - console.error(error); - } + console.error(error); if (error.status && error.status === 401 && storage) { // Unauthorized. diff --git a/client/coral-embed-stream/src/graphql/utils.js b/client/coral-embed-stream/src/graphql/utils.js index 8ab24d91f..0c46b45e3 100644 --- a/client/coral-embed-stream/src/graphql/utils.js +++ b/client/coral-embed-stream/src/graphql/utils.js @@ -141,18 +141,18 @@ export function findCommentWithId(nodes, id) { return findComment(nodes, (node) => node.id === id); } -export function findCommentInEmbedQuery(props, callbackOrId) { +export function findCommentInEmbedQuery(root, callbackOrId) { let callback = callbackOrId; if (typeof callbackOrId === 'string') { callback = (node) => node.id === callbackOrId; } - if (props.asset.comment) { - return findComment([getTopLevelParent(props.asset.comment)], callback); + if (root.asset.comment) { + return findComment([getTopLevelParent(root.asset.comment)], callback); } - if (!props.asset.comments) { + if (!root.asset.comments) { return false; } - return findComment(props.asset.comments.nodes, callback); + return findComment(root.asset.comments.nodes, callback); } function findAndInsertFetchedComments(parent, comments, parent_id) { diff --git a/client/coral-embed-stream/src/tabs/stream/containers/Stream.js b/client/coral-embed-stream/src/tabs/stream/containers/Stream.js index b4f375632..dad69b960 100644 --- a/client/coral-embed-stream/src/tabs/stream/containers/Stream.js +++ b/client/coral-embed-stream/src/tabs/stream/containers/Stream.js @@ -108,7 +108,7 @@ class StreamContainer extends React.Component { } loadNewReplies = (parent_id) => { - const comment = findCommentInEmbedQuery(this.props, parent_id); + const comment = findCommentInEmbedQuery(this.props.root, parent_id); return this.props.data.fetchMore({ query: LOAD_MORE_QUERY, diff --git a/client/coral-framework/graphql/anywhere.js b/client/coral-framework/graphql/anywhere.js deleted file mode 100644 index 7a93f16ea..000000000 --- a/client/coral-framework/graphql/anywhere.js +++ /dev/null @@ -1,23 +0,0 @@ -import graphql from '@coralproject/graphql-anywhere-optimized'; -import {createTypeGetter} from 'graphql-ast-tools'; -import introspectionData from './introspection.json'; - -// Use typeGetter to get more optimized documents. -const typeGetter = createTypeGetter(introspectionData); - -// Use global fragment cache for transformed fragments. -const fragmentMap = {}; - -export default (...args) => { - while (args.length < 7) { - args.push(undefined); - } - - const transformOptions = { - typeGetter, - fragmentMap, - }; - - args[6] = transformOptions; - return graphql(...args); -}; diff --git a/client/coral-framework/services/bootstrap.js b/client/coral-framework/services/bootstrap.js index 1328b169d..3ef6629c1 100644 --- a/client/coral-framework/services/bootstrap.js +++ b/client/coral-framework/services/bootstrap.js @@ -124,6 +124,10 @@ export async function createContext({ const plugins = createPluginsService(pluginsConfig); const graphql = createGraphQLService( createGraphQLRegistry(plugins.getSlotFragments.bind(plugins)), + { + introspectionData, + optimize: process.env.NODE_ENV === 'production', + }, ); if (!notification) { diff --git a/client/coral-framework/services/graphql.js b/client/coral-framework/services/graphql.js index d7b132bd4..fb40ecf71 100644 --- a/client/coral-framework/services/graphql.js +++ b/client/coral-framework/services/graphql.js @@ -1,3 +1,4 @@ +import {transformDocument, createTypeGetter} from 'graphql-ast-tools'; import {addTypenameToDocument} from 'apollo-client/queries/queryTransform'; /** @@ -5,7 +6,18 @@ import {addTypenameToDocument} from 'apollo-client/queries/queryTransform'; * @param {string} basename base path of the url * @return {Object} histor service */ -export function createGraphQLService(registry) { +export function createGraphQLService(registry, { + introspectionData, + optimize = false, +}) { + const transformOptions = { + typeGetter: optimize && introspectionData ? createTypeGetter(introspectionData) : null, + + // Use shared fragment map. + // Attention: Fragment names must be unique otherwise weird things will happen. + fragmentMap: {}, + }; + return { registry, resolveDocument(documentOrCallback, props, context) { @@ -14,6 +26,10 @@ export function createGraphQLService(registry) { : documentOrCallback; document = registry.resolveFragments(document); + if (optimize) { + document = transformDocument(document, transformOptions); + } + // We also add typenames to the document which apollo would usually do, // but we also use the network interface in subscriptions directly // which require the resolved typenames. diff --git a/package.json b/package.json index 1273015fe..4712a75c4 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "talk", - "version": "3.8.2", + "version": "3.8.1", "description": "A better commenting experience from Mozilla, The New York Times, and the Washington Post. https://coralproject.net", "main": "app.js", "private": true, @@ -106,7 +106,7 @@ "fs-extra": "^4.0.1", "gql-merge": "^0.0.4", "graphql": "^0.9.1", - "graphql-ast-tools": "0.2.3", + "graphql-ast-tools": "^0.1.5", "graphql-docs": "0.2.0", "graphql-errors": "^2.1.0", "graphql-redis-subscriptions": "1.3.0", diff --git a/webpack.config.js b/webpack.config.js index 14ac7caf5..60a49b6eb 100644 --- a/webpack.config.js +++ b/webpack.config.js @@ -120,7 +120,7 @@ const config = { }, resolve: { alias: { - 'graphql-anywhere': path.resolve(__dirname, 'client/coral-framework/graphql/anywhere'), + 'graphql-anywhere': '@coralproject/graphql-anywhere-optimized', 'plugin-api': path.resolve(__dirname, 'plugin-api/'), plugins: path.resolve(__dirname, 'plugins/'), pluginsConfig: pluginsPath diff --git a/yarn.lock b/yarn.lock index ea99f0cfa..c01e2e85a 100644 --- a/yarn.lock +++ b/yarn.lock @@ -12,10 +12,10 @@ eslint-plugin-react "^7.3.0" "@coralproject/graphql-anywhere-optimized@^0.1.0": - version "0.1.5" - resolved "https://registry.yarnpkg.com/@coralproject/graphql-anywhere-optimized/-/graphql-anywhere-optimized-0.1.5.tgz#67c862bf908ea717d9521ea76266b5bc9f109c65" + version "0.1.3" + resolved "https://registry.yarnpkg.com/@coralproject/graphql-anywhere-optimized/-/graphql-anywhere-optimized-0.1.3.tgz#f92f3906bb04f001aef725697237786752c49bd6" dependencies: - graphql-ast-tools "^0.2.2" + graphql-ast-tools "^0.1.6" "@kadira/storybook-deployer@^1.1.0": version "1.2.0" @@ -3521,9 +3521,9 @@ graphql-anywhere@^3.0.1: version "3.1.0" resolved "https://registry.yarnpkg.com/graphql-anywhere/-/graphql-anywhere-3.1.0.tgz#3ea0d8e8646b5cee68035016a9a7557c15c21e96" -graphql-ast-tools@0.2.3, graphql-ast-tools@^0.2.2: - version "0.2.3" - resolved "https://registry.yarnpkg.com/graphql-ast-tools/-/graphql-ast-tools-0.2.3.tgz#447fb05905ebb90f0a5bba81d5715249e9937135" +graphql-ast-tools@^0.1.5, graphql-ast-tools@^0.1.6: + version "0.1.6" + resolved "https://registry.yarnpkg.com/graphql-ast-tools/-/graphql-ast-tools-0.1.6.tgz#48eb656434bf4c7dba2a0d4784f1fdb988ab70ed" dependencies: apollo-utilities "^1.0.3"