feat(pipe): Simplify pipeline step executor

BREAKING CHANGE: return value from pipeline functions are no longer merged into the context

NOTE: Most of the functional changes live in src/pipeline.js;
most other changes are just refactoring the rest of the code
base to utilize the changes there.

feat(pipe): Drop write protection in pipeline fixes #228
feat(pipe): Get rid of parameter merging fixes #223

  This specific change necessitated the bulk of the changes to the code
  bases; most of the code that was refactored relied on parameter
  merging before.

feat(pipe): Fuller error reporting in pipeline

  Specifically, all errors are now reported by printing an
  error message the stack trace with additional information
  identifying the specific pipeline step.
  On the http level a 500 status code is transmitted.

Co-authored-by: Tobias Bocanegra <tripod@adobe.com>

.eslintrc.js

@@ -42,8 +42,12 @@ module.exports = {
     // Allow return before else & redundant else statements
     'no-else-return': 'off',
 
-    // allow dangling underscores for 'fields'
-    'no-underscore-dangle': ['error', {'allowAfterThis': true}],
+    // Quite useful to mark values as unused
+    'no-underscore-dangle': 'off',
+
+    // We have quite a lot of use cases where assignment to function
+    // parameters is definitely desirable
+    'no-param-reassign': 'off',
 
     // enforce license header
     'header/header': [2, 'block', ['',

src/defaults/html.pipe.js

@@ -18,7 +18,7 @@ const meta = require('../html/get-metadata.js');
 const html = require('../html/make-html.js');
 const responsive = require('../html/responsify-images.js');
 const type = require('../utils/set-content-type.js');
-const { selectStatus } = require('../html/set-status.js');
+const selectStatus = require('../html/set-status.js');
 const smartypants = require('../html/smartypants');
 const sections = require('../html/split-sections');
 const { selectstrain, selecttest } = require('../utils/conditional-sections');
@@ -36,7 +36,6 @@ const tohast = require('../html/html-to-hast');
 const tohtml = require('../html/stringify-hast');
 const addHeaders = require('../html/add-headers');
 
-/* eslint no-param-reassign: off */
 /* eslint newline-per-chained-call: off */
 
 function hascontent({ content }) {
@@ -71,7 +70,7 @@ const htmlpipe = (cont, payload, action) => {
     .after(addHeaders)
     .after(tohtml) // end HTML post-processing
     .after(flag).expose('esi').when(esi) // flag ESI when there is ESI in the response
-    .error(selectStatus(production()));
+    .error(selectStatus);
 
   action.logger.log('debug', 'Running HTML pipeline');
   return pipe.run(payload);

src/defaults/json.pipe.js

@@ -25,7 +25,6 @@ const emit = require('../json/emit-json.js');
 const { selectStatus } = require('../json/set-json-status.js');
 const parseFrontmatter = require('../html/parse-frontmatter.js');
 
-/* eslint no-param-reassign: off */
 /* eslint newline-per-chained-call: off */
 
 const jsonpipe = (cont, payload, action) => {

src/defaults/xml.pipe.js

@@ -25,11 +25,10 @@ const dump = require('../utils/dump-context.js');
 const validate = require('../utils/validate');
 const type = require('../utils/set-content-type.js');
 const emit = require('../xml/emit-xml.js');
-const { selectStatus } = require('../xml/set-xml-status.js');
+const selectStatus = require('../xml/set-xml-status.js');
 const check = require('../xml/check-xml');
 const parseFrontmatter = require('../html/parse-frontmatter');
 
-/* eslint no-param-reassign: off */
 /* eslint newline-per-chained-call: off */
 
 const xmlpipe = (cont, payload, action) => {
@@ -53,7 +52,7 @@ const xmlpipe = (cont, payload, action) => {
     .when(uncached)
     .after(key)
     .after(flag).expose('esi').when(esi) // flag ESI when there is ESI in the response
-    .error(selectStatus(production()));
+    .error(selectStatus);
 
   action.logger.log('debug', 'Running XML pipeline');
   return pipe.run(payload);

src/html/fetch-markdown.js

@@ -9,9 +9,10 @@
  * OF ANY KIND, either express or implied. See the License for the specific language
  * governing permissions and limitations under the License.
  */
+const { inspect } = require('util');
 const client = require('request-promise-native');
 const URI = require('uri-js');
-const { bail } = require('../helper');
+const { setdefault } = require('@adobe/helix-shared').types;
 
 function uri(root, owner, repo, ref, path) {
   const rootURI = URI.parse(root);
@@ -36,16 +37,11 @@ function uri(root, owner, repo, ref, path) {
  * @param {import("../context").Context} ctx some param
  * @param {import("../context").Action} action some other param
  */
-async function fetch(
-  { content: { sources = [] } = {} },
-  {
-    secrets = {},
-    request,
-    logger,
-  },
-) {
+async function fetch(context, { secrets = {}, request, logger }) {
+  const content = setdefault(context, 'content', {});
+
   if (!request || !request.params) {
-    return bail(logger, 'Request parameters are missing');
+    throw new Error('Request parameters missing');
   }
 
   let timeout;
@@ -64,13 +60,13 @@ async function fetch(
 
   // bail if a required parameter cannot be found
   if (!owner) {
-    return bail(logger, 'Unknown owner, cannot fetch content');
+    throw new Error('Unknown owner, cannot fetch content');
   }
   if (!repo) {
-    return bail(logger, 'Unknown repo, cannot fetch content');
+    throw new Error('Unknown repo, cannot fetch content');
   }
   if (!path) {
-    return bail(logger, 'Unknown path, cannot fetch content');
+    throw new Error('Unknown path, cannot fetch content');
   }
   if (!ref) {
     logger.warn(`Recoverable error: no ref given for ${repo}/${owner}.git${path}, falling back to master`);
@@ -86,24 +82,25 @@ async function fetch(
     timeout,
     time: true,
   };
+
   logger.debug(`fetching Markdown from ${options.uri}`);
   try {
-    const response = await client(options);
-    return {
-      content: {
-        body: response,
-        sources: [...sources, options.uri],
-      },
-    };
+    content.body = await client(options);
+    setdefault(content, 'sources', []).push(options.uri);
   } catch (e) {
-    if (e && e.statusCode && e.statusCode === 404) {
-      return bail(logger, `Could not find Markdown at ${options.uri}`, e, 404);
-    } else if ((e && e.response && e.response.elapsedTime && e.response.elapsedTime > timeout) || (e && e.cause && e.cause.code && (e.cause.code === 'ESOCKETTIMEDOUT' || e.cause.code === 'ETIMEDOUT'))) {
+    if (e.statusCode === 404) {
+      logger.error(`Could not find Markdown at ${options.uri}`);
+      setdefault(context, 'response', {}).status = 404;
+    } else if ((e.response && e.response.elapsedTime && e.response.elapsedTime > timeout) || (e.cause && e.cause.code && (e.cause.code === 'ESOCKETTIMEDOUT' || e.cause.code === 'ETIMEDOUT'))) {
       // return gateway timeout
-      return bail(logger, `Gateway timout of ${timeout} milliseconds exceeded for ${options.uri}`, e, 504);
+      logger.error(`Gateway timout of ${timeout} milliseconds exceeded for ${options.uri}`);
+      setdefault(context, 'response', {}).status = 504;
+    } else {
+      logger.error(`Error while fetching Markdown from ${options.uri} with the following `
+                   + `options:\n${inspect(options, { depth: null })}`);
+      setdefault(context, 'response', {}).status = 502;
     }
-    // return a bad gateway for all other errors
-    return bail(logger, `Could not fetch Markdown from ${options.uri}`, e, 502);
+    context.error = e;
   }
 }
 

src/html/find-embeds.js

@@ -9,7 +9,6 @@
  * OF ANY KIND, either express or implied. See the License for the specific language
  * governing permissions and limitations under the License.
  */
-/* eslint-disable no-param-reassign */
 const map = require('unist-util-map');
 const URI = require('uri-js');
 const mm = require('micromatch');
@@ -135,8 +134,6 @@ function find({ content: { mdast }, request: { extension, url } },
       internalembed(internalImgEmbed(node, url, contentext, resourceext), node, `.${EMBED_SELECTOR}.${extension}`);
     }
   });
-
-  return { content: { mdast } };
 }
 
 module.exports = find;

src/html/flag-esi.js

@@ -9,26 +9,29 @@
  * OF ANY KIND, either express or implied. See the License for the specific language
  * governing permissions and limitations under the License.
  */
+
+const { merge } = require('lodash');
+
 /**
  * Detects if ESI tags are used in the repose body. Intended to be used as
  * a predicate in the pipeline construction.
  * @param {Context} param0 the pipeline payload
  */
 function esi({ response }) {
-  return response && response.body && /<esi:include/.test(response.body);
+  return Boolean(response && response.body && /<esi:include/.test(response.body));
 }
 
 /**
  * Flags the response as containing ESI by adding the `X-ESI: enabled` header
  */
-function flag() {
-  return {
+function flag(context) {
+  merge(context, {
     response: {
       headers: {
         'X-ESI': 'enabled',
       },
     },
-  };
+  });
 }
 
 module.exports = {

src/html/get-metadata.js

@@ -11,19 +11,21 @@
  */
 const { select, selectAll } = require('unist-util-select');
 const plain = require('mdast-util-to-string');
-const { flat, obj, map } = require('@adobe/helix-shared').sequence;
+const { empty } = require('@adobe/helix-shared').types;
+const {
+  flat, obj, map, each,
+} = require('@adobe/helix-shared').sequence;
 
 
 function yaml(section) {
   const yamls = selectAll('yaml', section);
-  /* eslint-disable-next-line no-param-reassign */
   section.meta = obj(flat(map(yamls, ({ payload }) => payload)));
   return section;
 }
 
 function title(section) {
   const header = select('heading', section);
-  return header ? Object.assign({ title: plain(header) }, section) : section;
+  section.title = header ? plain(header) : '';
 }
 
 function intro(section) {
@@ -34,14 +36,16 @@ function intro(section) {
     }
     return true;
   })[0];
-  return para ? Object.assign({ intro: plain(para) }, section) : section;
+  section.intro = para ? plain(para) : '';
 }
 
 function image(section) {
   // selects the most prominent image of the section
   // TODO: get a better measure of prominence than "first"
   const img = select('image', section);
-  return img ? Object.assign({ image: img.url }, section) : section;
+  if (img) {
+    section.image = img.url;
+  }
 }
 
 /**
@@ -77,7 +81,6 @@ function sectiontype(section) {
   function reducer(counter, node) {
     const { type, children: pChildren } = node;
 
-    // eslint-disable-next-line no-param-reassign
     node.data = Object.assign({ types: [] }, node.data);
 
     if (type === 'yaml') {
@@ -129,43 +132,36 @@ function sectiontype(section) {
   }
 
   const typecounter = children.reduce(reducer, {});
-
-  const types = constructTypes(typecounter);
-
-  return Object.assign({ types }, section);
+  section.types = constructTypes(typecounter);
 }
 
 function fallback(section) {
   if (section.intro && !section.title) {
-    return Object.assign({ title: section.intro }, section);
+    section.title = section.intro;
   } else if (section.title && !section.intro) {
-    return Object.assign({ intro: section.title }, section);
+    section.intro = section.title;
   }
-  return section;
 }
 
-function getmetadata({ content: { sections = [] } }, { logger }) {
+function getmetadata({ content }, { logger }) {
+  const { sections } = content;
+  if (!sections) {
+    content.meta = {};
+    return;
+  }
+
   logger.debug(`Parsing Markdown Metadata from ${sections.length} sections`);
 
-  const retsections = sections
-    .map(yaml)
-    .map(title)
-    .map(intro)
-    .map(image)
-    .map(sectiontype)
-    .map(fallback);
-  const img = retsections.filter(section => section.image)[0];
-  if (retsections[0]) {
-    const retcontent = {
-      sections: retsections,
-      meta: retsections[0].meta,
-      title: retsections[0].title,
-      intro: retsections[0].intro,
-      image: img ? img.image : undefined,
-    };
-    return { content: retcontent };
-  }
-  return { content: { meta: {} } };
+  each([yaml, title, intro, image, sectiontype, fallback], (fn) => {
+    each(sections, fn);
+  });
+
+  const img = sections.filter(section => section.image)[0];
+
+  content.meta = empty(sections) ? {} : sections[0].meta;
+  content.title = empty(sections) ? '' : sections[0].title;
+  content.intro = empty(sections) ? '' : sections[0].intro;
+  content.image = img ? img.image : undefined;
 }
 
 module.exports = getmetadata;

src/html/html-to-hast.js

@@ -12,14 +12,9 @@
 const unified = require('unified');
 const parse = require('rehype-parse');
 
-function tohast({ response: { body } }) {
-  const fragment = !body.match(/<html/i);
-  const hast = unified().use(parse, { fragment }).parse(body);
-  return {
-    response: {
-      hast,
-    },
-  };
+function tohast({ response }) {
+  const fragment = !response.body.match(/<html/i);
+  response.hast = unified().use(parse, { fragment }).parse(response.body);
 }
 
 module.exports = tohast;

src/html/make-html.js

@@ -12,15 +12,14 @@
 const tohtast = require('mdast-util-to-hast');
 const VDOMTransformer = require('../utils/mdast-to-vdom');
 
-function html({ content: { mdast }, request }, { logger, secrets }) {
+function html({ content, request }, { logger, secrets }) {
+  const { mdast } = content;
   logger.log('debug', `Turning Markdown into HTML from ${typeof mdast}`);
   const extension = request && request.extension ? request.extension : 'html';
-  const content = {};
   // do we still need this?
   content.htast = tohtast(mdast);
   content.document = new VDOMTransformer(mdast, Object.assign({ extension }, secrets))
     .getDocument();
-  return { content };
 }
 
 module.exports = html;