From 5c007e3d53b9914f2ed71abd185434f53a9cd0a4 Mon Sep 17 00:00:00 2001 From: Stuart Taylor Date: Wed, 3 Jan 2018 23:38:52 +0000 Subject: [PATCH 1/6] feat(transformers): Disable JS in preview on error --- client/epics/err-epic.js | 4 +-- common/app/routes/Challenges/Preview.jsx | 3 +- .../routes/Challenges/rechallenge/builders.js | 16 ++++++--- .../Challenges/rechallenge/transformers.js | 35 +++++++++++++++++-- .../redux/execute-challenge-epic.js | 9 ++--- common/app/routes/Challenges/redux/index.js | 4 +++ common/app/routes/Challenges/utils/build.js | 10 ++++++ common/utils/polyvinyl.js | 25 ++++++++++--- 8 files changed, 86 insertions(+), 20 deletions(-) diff --git a/client/epics/err-epic.js b/client/epics/err-epic.js index 12c0689843..e94ce68ade 100644 --- a/client/epics/err-epic.js +++ b/client/epics/err-epic.js @@ -4,7 +4,7 @@ export default function errorSaga(actions) { return actions.filter(({ error }) => !!error) .map(({ error }) => error) .doOnNext(error => console.error(error)) - .map(() => makeToast({ - message: 'Something went wrong, please try again later' + .map(error => makeToast({ + message: error.message || 'Something went wrong, please try again later' })); } diff --git a/common/app/routes/Challenges/Preview.jsx b/common/app/routes/Challenges/Preview.jsx index a69b225fc8..0ca2decdbb 100644 --- a/common/app/routes/Challenges/Preview.jsx +++ b/common/app/routes/Challenges/Preview.jsx @@ -1,4 +1,5 @@ -import React, { PropTypes, PureComponent } from 'react'; +import React, { PureComponent } from 'react'; +import PropTypes from 'prop-types'; import { connect } from 'react-redux'; import ns from './ns.json'; diff --git a/common/app/routes/Challenges/rechallenge/builders.js b/common/app/routes/Challenges/rechallenge/builders.js index c38c1a6f30..d5f833953b 100644 --- a/common/app/routes/Challenges/rechallenge/builders.js +++ b/common/app/routes/Challenges/rechallenge/builders.js @@ -50,22 +50,28 @@ const wrapInStyle = partial(transformContents, (content) => ( const setExtToHTML = partial(setExt, 'html'); const padContentWithJsCatch = partial(compileHeadTail, jsCatch); const padContentWithHTMLCatch = partial(compileHeadTail, htmlCatch); +function transformErrorProtect(fn) { + return cond([ + [ matchesProperty('transformError', false), fn ], + [ stubTrue, identity ] + ]); +} -export const jsToHtml = cond([ +export const jsToHtml = transformErrorProtect(cond([ [ matchesProperty('ext', 'js'), flow(padContentWithJsCatch, wrapInScript, setExtToHTML) ], [ stubTrue, identity ] -]); +])); -export const cssToHtml = cond([ +export const cssToHtml = transformErrorProtect(cond([ [ matchesProperty('ext', 'css'), flow(padContentWithHTMLCatch, wrapInStyle, setExtToHTML) ], - [ stubTrue, identity ] -]); + [ stubTrue, identity ] +])); // FileStream::concactHtml( // required: [ ...Object ], diff --git a/common/app/routes/Challenges/rechallenge/transformers.js b/common/app/routes/Challenges/rechallenge/transformers.js index c0b3a9a94a..fb46b3cc6f 100644 --- a/common/app/routes/Challenges/rechallenge/transformers.js +++ b/common/app/routes/Challenges/rechallenge/transformers.js @@ -1,7 +1,9 @@ import { + attempt, cond, flow, identity, + isError, matchesProperty, overEvery, overSome, @@ -105,15 +107,42 @@ export const replaceNBSP = cond([ [ stubTrue, identity ] ]); +const transformErrorWarn = '/* __fcc--TransformError__ */'; + +function tryJSTransform(wrap = identity, defaultAssignment = 'code') { + return function transformWrappedPoly(source) { + const result = attempt(wrap, source); + if (isError(result)) { + const friendlyError = `${result}` + .match(/[\w\W]+?\n/)[0] + .replace(' unknown:', ''); + console.error(friendlyError); + return `var ${defaultAssignment} = null; ${transformErrorWarn}`; + } + return result; + }; +} + +function checkForTransformError(file) { + const potentialError = file.contents.includes(transformErrorWarn); + if (potentialError) { + return { + ...vinyl.setTransformError(true, file) + }; + } + return file; +} + export const babelTransformer = cond([ [ testJS$JSX, flow( partial( vinyl.transformHeadTailAndContents, - babelTransformCode + tryJSTransform(babelTransformCode, 'JSX') ), - partial(vinyl.setExt, 'js') + partial(vinyl.setExt, 'js'), + checkForTransformError ) ], [ stubTrue, identity ] @@ -143,5 +172,5 @@ export function applyTransformers(file, transformers = _transformers) { return obs.flatMap(file => castToObservable(transformer(file))); }, Observable.of(file) - ); + ); } diff --git a/common/app/routes/Challenges/redux/execute-challenge-epic.js b/common/app/routes/Challenges/redux/execute-challenge-epic.js index 10e24f9269..b3ee5a2b3a 100644 --- a/common/app/routes/Challenges/redux/execute-challenge-epic.js +++ b/common/app/routes/Challenges/redux/execute-challenge-epic.js @@ -12,7 +12,8 @@ import { codeLockedSelector, showPreviewSelector, - testsSelector + testsSelector, + disableJSOnError } from './'; import { buildFromFiles, @@ -26,7 +27,8 @@ import { import { createErrorObservable, - challengeSelector + challengeSelector, + doActionOnError } from '../../../redux'; @@ -40,7 +42,6 @@ export function updateMainEpic(actions, { getState }, { document }) { const proxyLogger = new Subject(); const frameMain = createMainFramer(document, getState, proxyLogger); const buildAndFrameMain = actions::ofType( - types.unlockUntrustedCode, types.modernEditorUpdated, types.classicEditorUpdated, types.executeChallenge, @@ -55,7 +56,7 @@ export function updateMainEpic(actions, { getState }, { document }) { .flatMapLatest(() => buildFromFiles(getState(), true) .map(frameMain) .ignoreElements() - .catch(createErrorObservable) + .catch(doActionOnError(() => disableJSOnError())) ); return Observable.merge(buildAndFrameMain, proxyLogger.map(updateOutput)); }); diff --git a/common/app/routes/Challenges/redux/index.js b/common/app/routes/Challenges/redux/index.js index 090de13004..05b34be631 100644 --- a/common/app/routes/Challenges/redux/index.js +++ b/common/app/routes/Challenges/redux/index.js @@ -77,6 +77,7 @@ export const types = createTypes([ 'checkChallenge', createAsyncTypes('submitChallenge'), 'moveToNextChallenge', + 'disableJSOnError', // help 'openHelpModal', @@ -151,6 +152,8 @@ export const submitChallengeComplete = createAction( export const moveToNextChallenge = createAction(types.moveToNextChallenge); +export const disableJSOnError = createAction(types.disableJSOnError); + // help export const openHelpModal = createAction(types.openHelpModal); export const closeHelpModal = createAction(types.closeHelpModal); @@ -306,6 +309,7 @@ export default combineReducers( [ combineActions( types.classicEditorUpdated, + types.disableJSOnError, types.modernEditorUpdated ) ]: state => ({ diff --git a/common/app/routes/Challenges/utils/build.js b/common/app/routes/Challenges/utils/build.js index a1b9d02424..dc3d4455b2 100644 --- a/common/app/routes/Challenges/utils/build.js +++ b/common/app/routes/Challenges/utils/build.js @@ -42,6 +42,15 @@ const globalRequires = [ jQuery ]; +function postTransformCheck(file) { + if (file.transformError) { + // this will enable us to tap into the dipatch pipeline + // and disable JS in the preview + throw new Error('There was an error whilst transforming your code'); + } + return file; +} + export function buildFromFiles(state, shouldProxyConsole) { const files = filesSelector(state); const required = challengeRequiredSelector(state); @@ -49,6 +58,7 @@ export function buildFromFiles(state, shouldProxyConsole) { return createFileStream(files) ::pipe(throwers) ::pipe(applyTransformers) + ::pipe(postTransformCheck) ::pipe(shouldProxyConsole ? proxyLoggerTransformer : identity) ::pipe(jsToHtml) ::pipe(cssToHtml) diff --git a/common/utils/polyvinyl.js b/common/utils/polyvinyl.js index 881b2709be..b0f060d0e5 100644 --- a/common/utils/polyvinyl.js +++ b/common/utils/polyvinyl.js @@ -77,7 +77,8 @@ export function createPoly({ path: name + '.' + ext, key: name + ext, contents, - error: null + error: null, + transformError: false }; } @@ -116,7 +117,7 @@ export function setContent(contents, poly) { }; } -// setExt(contents: String, poly: PolyVinyl) => PolyVinyl +// setExt(ext: String, poly: PolyVinyl) => PolyVinyl export function setExt(ext, poly) { checkPoly(poly); const newPoly = { @@ -129,7 +130,7 @@ export function setExt(ext, poly) { return newPoly; } -// setName(contents: String, poly: PolyVinyl) => PolyVinyl +// setName(name: String, poly: PolyVinyl) => PolyVinyl export function setName(name, poly) { checkPoly(poly); const newPoly = { @@ -142,7 +143,7 @@ export function setName(name, poly) { return newPoly; } -// setError(contents: String, poly: PolyVinyl) => PolyVinyl +// setError(error: Object, poly: PolyVinyl) => PolyVinyl export function setError(error, poly) { invariant( typeof error === 'object', @@ -155,6 +156,19 @@ export function setError(error, poly) { error }; } +// setTransformError(transformError: Boolean, poly: PolyVinyl) => PolyVinyl +export function setTransformError(transformError, poly) { + invariant( + typeof transformError === 'boolean', + 'transformError must be a boolean, but got %', + transformError + ); + checkPoly(poly); + return { + ...poly, + transformError + }; +} // clearHeadTail(poly: PolyVinyl) => PolyVinyl export function clearHeadTail(poly) { @@ -166,6 +180,7 @@ export function clearHeadTail(poly) { }; } +// appendToTail (tail: String, poly: PolyVinyl) => PolyVinyl export function appendToTail(tail, poly) { checkPoly(poly); return { @@ -174,7 +189,7 @@ export function appendToTail(tail, poly) { }; } -// compileHeadTail(contents: String, poly: PolyVinyl) => PolyVinyl +// compileHeadTail(padding: String, poly: PolyVinyl) => PolyVinyl export function compileHeadTail(padding = '', poly) { return clearHeadTail(transformContents( () => [ poly.head, poly.contents, poly.tail ].join(padding), From 303ad38b8e14ae3c141d31ef20ff94bf8a33c445 Mon Sep 17 00:00:00 2001 From: Stuart Taylor Date: Mon, 8 Jan 2018 00:00:34 +0000 Subject: [PATCH 2/6] feat(rechallenge): Remove JS files from build pipline if JS is disabled --- .../routes/Challenges/rechallenge/builders.js | 14 +++------- .../Challenges/rechallenge/transformers.js | 24 ++++------------ .../redux/execute-challenge-epic.js | 2 +- common/app/routes/Challenges/utils/build.js | 28 +++++++++++-------- common/utils/polyvinyl.js | 22 +++------------ 5 files changed, 31 insertions(+), 59 deletions(-) diff --git a/common/app/routes/Challenges/rechallenge/builders.js b/common/app/routes/Challenges/rechallenge/builders.js index d5f833953b..4863d6b94f 100644 --- a/common/app/routes/Challenges/rechallenge/builders.js +++ b/common/app/routes/Challenges/rechallenge/builders.js @@ -50,28 +50,22 @@ const wrapInStyle = partial(transformContents, (content) => ( const setExtToHTML = partial(setExt, 'html'); const padContentWithJsCatch = partial(compileHeadTail, jsCatch); const padContentWithHTMLCatch = partial(compileHeadTail, htmlCatch); -function transformErrorProtect(fn) { - return cond([ - [ matchesProperty('transformError', false), fn ], - [ stubTrue, identity ] - ]); -} -export const jsToHtml = transformErrorProtect(cond([ +export const jsToHtml = cond([ [ matchesProperty('ext', 'js'), flow(padContentWithJsCatch, wrapInScript, setExtToHTML) ], [ stubTrue, identity ] -])); +]); -export const cssToHtml = transformErrorProtect(cond([ +export const cssToHtml = cond([ [ matchesProperty('ext', 'css'), flow(padContentWithHTMLCatch, wrapInStyle, setExtToHTML) ], [ stubTrue, identity ] -])); +]); // FileStream::concactHtml( // required: [ ...Object ], diff --git a/common/app/routes/Challenges/rechallenge/transformers.js b/common/app/routes/Challenges/rechallenge/transformers.js index fb46b3cc6f..60b584bc68 100644 --- a/common/app/routes/Challenges/rechallenge/transformers.js +++ b/common/app/routes/Challenges/rechallenge/transformers.js @@ -37,7 +37,7 @@ const NBSPReg = new RegExp(String.fromCharCode(160), 'g'); const isJS = matchesProperty('ext', 'js'); const testHTMLJS = overSome(isJS, matchesProperty('ext', 'html')); -const testJS$JSX = overSome(isJS, matchesProperty('ext', 'jsx')); +export const testJS$JSX = overSome(isJS, matchesProperty('ext', 'jsx')); // work around the absence of multi-flile editing // this can be replaced with `matchesProperty('ext', 'sass')` @@ -107,42 +107,28 @@ export const replaceNBSP = cond([ [ stubTrue, identity ] ]); -const transformErrorWarn = '/* __fcc--TransformError__ */'; - -function tryJSTransform(wrap = identity, defaultAssignment = 'code') { +function tryTransform(wrap = identity) { return function transformWrappedPoly(source) { const result = attempt(wrap, source); if (isError(result)) { const friendlyError = `${result}` .match(/[\w\W]+?\n/)[0] .replace(' unknown:', ''); - console.error(friendlyError); - return `var ${defaultAssignment} = null; ${transformErrorWarn}`; + throw new Error(friendlyError); } return result; }; } -function checkForTransformError(file) { - const potentialError = file.contents.includes(transformErrorWarn); - if (potentialError) { - return { - ...vinyl.setTransformError(true, file) - }; - } - return file; -} - export const babelTransformer = cond([ [ testJS$JSX, flow( partial( vinyl.transformHeadTailAndContents, - tryJSTransform(babelTransformCode, 'JSX') + tryTransform(babelTransformCode) ), - partial(vinyl.setExt, 'js'), - checkForTransformError + partial(vinyl.setExt, 'js') ) ], [ stubTrue, identity ] diff --git a/common/app/routes/Challenges/redux/execute-challenge-epic.js b/common/app/routes/Challenges/redux/execute-challenge-epic.js index b3ee5a2b3a..9913711ebd 100644 --- a/common/app/routes/Challenges/redux/execute-challenge-epic.js +++ b/common/app/routes/Challenges/redux/execute-challenge-epic.js @@ -42,6 +42,7 @@ export function updateMainEpic(actions, { getState }, { document }) { const proxyLogger = new Subject(); const frameMain = createMainFramer(document, getState, proxyLogger); const buildAndFrameMain = actions::ofType( + types.unlockUntrustedCode, types.modernEditorUpdated, types.classicEditorUpdated, types.executeChallenge, @@ -62,7 +63,6 @@ export function updateMainEpic(actions, { getState }, { document }) { }); } - export function executeChallengeEpic(actions, { getState }, { document }) { return Observable.of(document) // if document is not defined then none of this epic will run diff --git a/common/app/routes/Challenges/utils/build.js b/common/app/routes/Challenges/utils/build.js index dc3d4455b2..61a196abf5 100644 --- a/common/app/routes/Challenges/utils/build.js +++ b/common/app/routes/Challenges/utils/build.js @@ -6,11 +6,13 @@ import throwers from '../rechallenge/throwers'; import { backendFormValuesSelector, challengeTemplateSelector, - challengeRequiredSelector + challengeRequiredSelector, + isJSEnabledSelector } from '../redux'; import { applyTransformers, - proxyLoggerTransformer + proxyLoggerTransformer, + testJS$JSX } from '../rechallenge/transformers'; import { cssToHtml, @@ -42,23 +44,27 @@ const globalRequires = [ jQuery ]; -function postTransformCheck(file) { - if (file.transformError) { - // this will enable us to tap into the dipatch pipeline - // and disable JS in the preview - throw new Error('There was an error whilst transforming your code'); - } - return file; +function filterJSIfDisabled(state) { + const isJSEnabled = isJSEnabledSelector(state); + return file => { + if (testJS$JSX(file) && !isJSEnabled) { + return null; + } + return file; + }; } export function buildFromFiles(state, shouldProxyConsole) { const files = filesSelector(state); const required = challengeRequiredSelector(state); const finalRequires = [...globalRequires, ...required ]; - return createFileStream(files) + const requiredFiles = Object.keys(files) + .map(key => files[key]) + .filter(filterJSIfDisabled(state)) + .filter(Boolean); + return createFileStream(requiredFiles) ::pipe(throwers) ::pipe(applyTransformers) - ::pipe(postTransformCheck) ::pipe(shouldProxyConsole ? proxyLoggerTransformer : identity) ::pipe(jsToHtml) ::pipe(cssToHtml) diff --git a/common/utils/polyvinyl.js b/common/utils/polyvinyl.js index b0f060d0e5..47f1f4b763 100644 --- a/common/utils/polyvinyl.js +++ b/common/utils/polyvinyl.js @@ -5,11 +5,11 @@ import castToObservable from '../app/utils/cast-to-observable.js'; // createFileStream( -// files: Dictionary[Path, PolyVinyl] +// files: List[ PolyVinyl ] // ) => Observable[...Observable[...PolyVinyl]] -export function createFileStream(files = {}) { +export function createFileStream(files = []) { return Observable.of( - Observable.from(Object.keys(files).map(key => files[key])) + Observable.from(files) ); } @@ -77,8 +77,7 @@ export function createPoly({ path: name + '.' + ext, key: name + ext, contents, - error: null, - transformError: false + error: null }; } @@ -156,19 +155,6 @@ export function setError(error, poly) { error }; } -// setTransformError(transformError: Boolean, poly: PolyVinyl) => PolyVinyl -export function setTransformError(transformError, poly) { - invariant( - typeof transformError === 'boolean', - 'transformError must be a boolean, but got %', - transformError - ); - checkPoly(poly); - return { - ...poly, - transformError - }; -} // clearHeadTail(poly: PolyVinyl) => PolyVinyl export function clearHeadTail(poly) { From b2a70fd5baed4aea8500e89a09e785339113cf48 Mon Sep 17 00:00:00 2001 From: Stuart Taylor Date: Mon, 8 Jan 2018 00:09:08 +0000 Subject: [PATCH 3/6] Remove weird white space not present locally --- common/app/routes/Challenges/rechallenge/builders.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/app/routes/Challenges/rechallenge/builders.js b/common/app/routes/Challenges/rechallenge/builders.js index 4863d6b94f..c38c1a6f30 100644 --- a/common/app/routes/Challenges/rechallenge/builders.js +++ b/common/app/routes/Challenges/rechallenge/builders.js @@ -64,7 +64,7 @@ export const cssToHtml = cond([ matchesProperty('ext', 'css'), flow(padContentWithHTMLCatch, wrapInStyle, setExtToHTML) ], - [ stubTrue, identity ] + [ stubTrue, identity ] ]); // FileStream::concactHtml( From 11821e40aa6ab6985da39198fe7845984732b72e Mon Sep 17 00:00:00 2001 From: Stuart Taylor Date: Mon, 8 Jan 2018 00:09:55 +0000 Subject: [PATCH 4/6] Remove weird white space not present locally --- common/app/routes/Challenges/rechallenge/transformers.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/app/routes/Challenges/rechallenge/transformers.js b/common/app/routes/Challenges/rechallenge/transformers.js index 60b584bc68..82c55e5141 100644 --- a/common/app/routes/Challenges/rechallenge/transformers.js +++ b/common/app/routes/Challenges/rechallenge/transformers.js @@ -158,5 +158,5 @@ export function applyTransformers(file, transformers = _transformers) { return obs.flatMap(file => castToObservable(transformer(file))); }, Observable.of(file) - ); + ); } From 42f9b6fc150fb35ff5c5cddb768f18685c1146f0 Mon Sep 17 00:00:00 2001 From: Stuart Taylor Date: Tue, 16 Jan 2018 18:09:29 +0000 Subject: [PATCH 5/6] Remove error.message from Toast --- client/epics/err-epic.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/client/epics/err-epic.js b/client/epics/err-epic.js index e94ce68ade..12c0689843 100644 --- a/client/epics/err-epic.js +++ b/client/epics/err-epic.js @@ -4,7 +4,7 @@ export default function errorSaga(actions) { return actions.filter(({ error }) => !!error) .map(({ error }) => error) .doOnNext(error => console.error(error)) - .map(error => makeToast({ - message: error.message || 'Something went wrong, please try again later' + .map(() => makeToast({ + message: 'Something went wrong, please try again later' })); } From ca7790158edcf931fc4b6658993efc4e2abf83ea Mon Sep 17 00:00:00 2001 From: Stuart Taylor Date: Wed, 17 Jan 2018 11:08:55 +0000 Subject: [PATCH 6/6] Use correct comment format --- common/utils/polyvinyl.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/utils/polyvinyl.js b/common/utils/polyvinyl.js index 47f1f4b763..18e5b15ea7 100644 --- a/common/utils/polyvinyl.js +++ b/common/utils/polyvinyl.js @@ -5,7 +5,7 @@ import castToObservable from '../app/utils/cast-to-observable.js'; // createFileStream( -// files: List[ PolyVinyl ] +// files: [...PolyVinyl] // ) => Observable[...Observable[...PolyVinyl]] export function createFileStream(files = []) { return Observable.of(