From beecb04c1aa57dae9b24d238a3d0b94d519d4a14 Mon Sep 17 00:00:00 2001
From: Oliver Eyton-Williams <ojeytonwilliams@gmail.com>
Date: Thu, 7 Nov 2019 14:35:17 +0100
Subject: [PATCH] fix: report errors thrown after the frame is ready

Certain challenges involve code that is not run until the user
interacts with the preview (typically via a click listener). This uses
consoleProxy to report those errors.

Error logging has been simplified, reducing the number of places errors
can be reported from.

Some of the redux-saga code has been renamed in an attempt to improve
clarity.
---
 .../Challenges/rechallenge/transformers.js    |  1 -
 .../redux/execute-challenge-saga.js           | 49 ++++++++++-----
 .../src/templates/Challenges/redux/index.js   |  3 +-
 .../src/templates/Challenges/utils/build.js   |  2 +-
 .../src/templates/Challenges/utils/frame.js   | 62 ++++++++++++-------
 5 files changed, 77 insertions(+), 40 deletions(-)

diff --git a/client/src/templates/Challenges/rechallenge/transformers.js b/client/src/templates/Challenges/rechallenge/transformers.js
index 1618c8ba4f..bb80a89ae0 100644
--- a/client/src/templates/Challenges/rechallenge/transformers.js
+++ b/client/src/templates/Challenges/rechallenge/transformers.js
@@ -59,7 +59,6 @@ function tryTransform(wrap = identity) {
   return function transformWrappedPoly(source) {
     const result = attempt(wrap, source);
     if (isError(result)) {
-      console.error(result);
       // note(Bouncey): Error thrown here to collapse the build pipeline
       // At the minute, it will not bubble up
       // We collapse the pipeline so the app doesn't fall over trying
diff --git a/client/src/templates/Challenges/redux/execute-challenge-saga.js b/client/src/templates/Challenges/redux/execute-challenge-saga.js
index a348776b48..0541f86900 100644
--- a/client/src/templates/Challenges/redux/execute-challenge-saga.js
+++ b/client/src/templates/Challenges/redux/execute-challenge-saga.js
@@ -50,7 +50,7 @@ export function* executeChallengeSaga() {
     );
     yield put(updateTests(tests));
 
-    yield fork(logToConsole, consoleProxy);
+    yield fork(takeEveryLog, consoleProxy);
     const proxyLogger = args => consoleProxy.put(args);
 
     const challengeData = yield select(challengeDataSelector);
@@ -59,7 +59,7 @@ export function* executeChallengeSaga() {
     const testRunner = yield call(
       getTestRunner,
       buildData,
-      proxyLogger,
+      { proxyLogger },
       document
     );
     const testResults = yield executeTests(testRunner, tests);
@@ -74,19 +74,28 @@ export function* executeChallengeSaga() {
   }
 }
 
-function* logToConsole(channel) {
+function* takeEveryLog(channel) {
+  // TODO: move all stringifying and escaping into the reducer so there is a
+  // single place responsible for formatting the logs.
   yield takeEvery(channel, function*(args) {
     yield put(updateLogs(escape(args)));
   });
 }
 
+function* takeEveryConsole(channel) {
+  // TODO: move all stringifying and escaping into the reducer so there is a
+  // single place responsible for formatting the console output.
+  yield takeEvery(channel, function*(args) {
+    yield put(updateConsole(escape(args)));
+  });
+}
+
 function* buildChallengeData(challengeData) {
   try {
     return yield call(buildChallenge, challengeData);
   } catch (e) {
-    yield put(disableBuildOnError(e));
-    // eslint-disable-next-line no-throw-literal
-    throw 'Build failed';
+    yield put(disableBuildOnError());
+    throw e;
   }
 }
 
@@ -136,30 +145,40 @@ function* previewChallengeSaga() {
     return;
   }
 
+  const logProxy = yield channel();
   const consoleProxy = yield channel();
+  const proxyLogger = args => logProxy.put(args);
+  const proxyUpdateConsole = args => consoleProxy.put(args);
 
   try {
     yield put(initLogs());
-    yield fork(logToConsole, consoleProxy);
-    const proxyLogger = args => consoleProxy.put(args);
-    const challengeData = yield select(challengeDataSelector);
+    yield fork(takeEveryLog, logProxy);
+    yield fork(takeEveryConsole, consoleProxy);
 
+    const challengeData = yield select(challengeDataSelector);
     const buildData = yield buildChallengeData(challengeData);
     // evaluate the user code in the preview frame or in the worker
     if (challengeHasPreview(challengeData)) {
       const document = yield getContext('document');
-      yield call(updatePreview, buildData, document, proxyLogger);
+      yield call(updatePreview, buildData, document, {
+        proxyLogger,
+        proxyUpdateConsole
+      });
     } else if (isJavaScriptChallenge(challengeData)) {
-      const runUserCode = getTestRunner(buildData, proxyLogger);
+      const runUserCode = getTestRunner(buildData, { proxyLogger });
       // without a testString the testRunner just evaluates the user's code
       yield call(runUserCode, null, 5000);
     }
+  } catch (err) {
+    console.log(err);
+    yield put(updateLogs(escape(err)));
+  } finally {
+    // consoleProxy is left open to record any errors triggered by user
+    // input.
+    logProxy.close();
+
     // To avoid seeing the default console, initialise and output in one call.
     yield all([put(initConsole('')), put(logsToConsole('// console output'))]);
-  } catch (err) {
-    console.error(err);
-  } finally {
-    consoleProxy.close();
   }
 }
 
diff --git a/client/src/templates/Challenges/redux/index.js b/client/src/templates/Challenges/redux/index.js
index cf9228df0f..dc075108f7 100644
--- a/client/src/templates/Challenges/redux/index.js
+++ b/client/src/templates/Challenges/redux/index.js
@@ -330,9 +330,8 @@ export const reducer = handleActions(
       isBuildEnabled: true,
       isCodeLocked: false
     }),
-    [types.disableBuildOnError]: (state, { payload }) => ({
+    [types.disableBuildOnError]: state => ({
       ...state,
-      consoleOut: state.consoleOut + ' \n' + payload,
       isBuildEnabled: false
     }),
 
diff --git a/client/src/templates/Challenges/utils/build.js b/client/src/templates/Challenges/utils/build.js
index 10300ad72a..cc2529c18f 100644
--- a/client/src/templates/Challenges/utils/build.js
+++ b/client/src/templates/Challenges/utils/build.js
@@ -85,7 +85,7 @@ const testRunners = {
   [challengeTypes.html]: getDOMTestRunner,
   [challengeTypes.backend]: getDOMTestRunner
 };
-export function getTestRunner(buildData, proxyLogger, document) {
+export function getTestRunner(buildData, { proxyLogger }, document) {
   const { challengeType } = buildData;
   const testRunner = testRunners[challengeType];
   if (testRunner) {
diff --git a/client/src/templates/Challenges/utils/frame.js b/client/src/templates/Challenges/utils/frame.js
index 63349e78d8..a7f5d3cd83 100644
--- a/client/src/templates/Challenges/utils/frame.js
+++ b/client/src/templates/Challenges/utils/frame.js
@@ -11,17 +11,14 @@ const testId = 'fcc-test-frame';
 // append to the current challenge url
 // this also allows in-page anchors to work properly
 // rather than load another instance of the learn
-//
-// if an error occurs during initialization
-// the __err prop will be set
-// This is then picked up in client/frame-runner.js during
-// runTestsInTestFrame below
+
+// window.onerror is added here to catch any errors thrown during the building
+// of the frame.
 const createHeader = (id = mainId) => `
   <base href='' />
   <script>
     window.__frameId = '${id}';
-    window.onerror = function(msg, url, ln, col, err) {
-      window.__err = err;
+    window.onerror = function(msg) {
       console.log(msg);
       return true;
     };
@@ -91,8 +88,7 @@ const buildProxyConsole = proxyLogger => ctx => {
 };
 
 const initTestFrame = frameReady => ctx => {
-  const contentLoaded = new Promise(resolve => waitForFrame(resolve)(ctx));
-  contentLoaded.then(async () => {
+  waitForFrame(ctx).then(async () => {
     const { sources, loadEnzyme } = ctx;
     // default for classic challenges
     // should not be used for modern
@@ -105,15 +101,33 @@ const initTestFrame = frameReady => ctx => {
   return ctx;
 };
 
-const waitForFrame = frameReady => ctx => {
-  if (ctx.document.readyState === 'loading') {
-    ctx.document.addEventListener('DOMContentLoaded', frameReady);
-  } else {
+const initMainFrame = (frameReady, proxyUpdateConsole) => ctx => {
+  waitForFrame(ctx).then(() => {
+    // Overwriting the onerror added by createHeader to catch any errors thrown
+    // after the frame is ready. It has to be overwritten, as proxyUpdateConsole
+    // cannot be added as part of createHeader.
+    ctx.window.onerror = function(msg) {
+      console.log(msg);
+      if (proxyUpdateConsole) {
+        proxyUpdateConsole(msg);
+      }
+      return true;
+    };
     frameReady();
-  }
+  });
   return ctx;
 };
 
+const waitForFrame = ctx => {
+  return new Promise(resolve => {
+    if (ctx.document.readyState === 'loading') {
+      ctx.document.addEventListener('DOMContentLoaded', resolve);
+    } else {
+      resolve();
+    }
+  });
+};
+
 function writeToFrame(content, frame) {
   frame.open();
   frame.write(content);
@@ -126,17 +140,23 @@ const writeContentToFrame = ctx => {
   return ctx;
 };
 
-export const createMainFramer = (document, frameReady, proxyConsole) =>
-  createFramer(document, frameReady, proxyConsole, mainId, waitForFrame);
+export const createMainFramer = (document, frameReady, proxy) =>
+  createFramer(document, frameReady, proxy, mainId, initMainFrame);
 
-export const createTestFramer = (document, frameReady, proxyConsole) =>
-  createFramer(document, frameReady, proxyConsole, testId, initTestFrame);
+export const createTestFramer = (document, frameReady, proxy) =>
+  createFramer(document, frameReady, proxy, testId, initTestFrame);
 
-const createFramer = (document, frameReady, proxyConsole, id, init) =>
+const createFramer = (
+  document,
+  frameReady,
+  { proxyLogger, proxyUpdateConsole },
+  id,
+  init
+) =>
   flow(
     createFrame(document, id),
     mountFrame(document),
+    buildProxyConsole(proxyLogger),
     writeContentToFrame,
-    buildProxyConsole(proxyConsole),
-    init(frameReady)
+    init(frameReady, proxyUpdateConsole)
   );