From b1fb6adc39e8dcddf5d88a1b11b3d3d8052ef3ab Mon Sep 17 00:00:00 2001 From: Oliver Eyton-Williams Date: Mon, 24 Jan 2022 19:02:25 +0100 Subject: [PATCH] fix: insert user html more consistently (#42195) * fix: use an iframe to preserve head and body * fix: remove unnecessary parsing of html The contents gets inserted into the DOM during transformHtml, which is always part of the build pipeline * fix: pipe contents through iframe * refactor: use the same code for both transforms * fix: try to handle test errors better Co-authored-by: moT01 <20648924+moT01@users.noreply.github.com> --- .../Challenges/rechallenge/builders.js | 11 +- .../Challenges/rechallenge/transformers.js | 117 +++++++++++------- .../step-002.md | 4 +- .../step-003.md | 6 +- .../step-005.md | 6 +- curriculum/package.json | 2 +- curriculum/test/test-challenges.js | 8 ++ 7 files changed, 92 insertions(+), 62 deletions(-) diff --git a/client/src/templates/Challenges/rechallenge/builders.js b/client/src/templates/Challenges/rechallenge/builders.js index 0b9ff12ab2..8bcc5f270c 100644 --- a/client/src/templates/Challenges/rechallenge/builders.js +++ b/client/src/templates/Challenges/rechallenge/builders.js @@ -14,13 +14,6 @@ import { transformContents } from '../../../../../utils/polyvinyl'; -const defaultTemplate = ({ source }) => { - return ` - - ${source} - `; -}; - const wrapInScript = partial( transformContents, content => `` @@ -67,7 +60,7 @@ export function concatHtml({ template, challengeFiles = [] } = {}) { - const createBody = template ? _template(template) : defaultTemplate; + const embedSource = template ? _template(template) : ({ source }) => source; const head = required .map(({ link, src }) => { if (link && src) { @@ -99,5 +92,5 @@ A required file can not have both a src and a link: src = ${src}, link = ${link} } }, ''); - return `${head}${createBody({ source })}`; + return `${head}${embedSource({ source })}`; } diff --git a/client/src/templates/Challenges/rechallenge/transformers.js b/client/src/templates/Challenges/rechallenge/transformers.js index ac43954bd3..213bec1297 100644 --- a/client/src/templates/Challenges/rechallenge/transformers.js +++ b/client/src/templates/Challenges/rechallenge/transformers.js @@ -180,10 +180,12 @@ function getBabelOptions({ preview = false, protect = true }) { } const sassWorker = createWorker(sassCompile); -async function transformSASS(element) { +async function transformSASS(contentDocument) { // we only teach scss syntax, not sass. Also the compiler does not seem to be // able to deal with sass. - const styleTags = element.querySelectorAll('style[type~="text/scss"]'); + const styleTags = contentDocument.querySelectorAll( + 'style[type~="text/scss"]' + ); await Promise.all( [].map.call(styleTags, async style => { style.type = 'text/css'; @@ -192,10 +194,10 @@ async function transformSASS(element) { ); } -async function transformScript(element) { +async function transformScript(contentDocument) { await loadBabel(); await loadPresetEnv(); - const scriptTags = element.querySelectorAll('script'); + const scriptTags = contentDocument.querySelectorAll('script'); scriptTags.forEach(script => { script.innerHTML = tryTransform(babelTransformCode(babelOptionsJS))( script.innerHTML @@ -203,59 +205,86 @@ async function transformScript(element) { }); } -const transformHtml = async function (file) { - const div = document.createElement('div'); - div.innerHTML = file.contents; - await Promise.all([transformSASS(div), transformScript(div)]); - return transformContents(() => div.innerHTML, file); -}; - // Find if the base html refers to the css or js files and record if they do. If // the link or script exists we remove those elements since those files don't // exist on the site, only in the editor -const transformIncludes = async function (fileP) { +const addImportedFiles = async function (fileP) { const file = await fileP; - const div = document.createElement('div'); - div.innerHTML = file.contents; - const link = - div.querySelector('link[href="styles.css"]') ?? - div.querySelector('link[href="./styles.css"]'); - const script = - div.querySelector('script[src="script.js"]') ?? - div.querySelector('script[src="./script.js"]'); - const importedFiles = []; - if (link) { - importedFiles.push('styles.css'); - link.remove(); - } - if (script) { - importedFiles.push('script.js'); - script.remove(); - } + const transform = frame => { + const documentElement = frame.contentDocument.documentElement; + const link = + documentElement.querySelector('link[href="styles.css"]') ?? + documentElement.querySelector('link[href="./styles.css"]'); + const script = + documentElement.querySelector('script[src="script.js"]') ?? + documentElement.querySelector('script[src="./script.js"]'); + const importedFiles = []; + if (link) { + importedFiles.push('styles.css'); + link.remove(); + } + if (script) { + importedFiles.push('script.js'); + script.remove(); + } + return { + contents: documentElement.innerHTML, + importedFiles + }; + }; + + const { importedFiles, contents } = await transformWithFrame( + transform, + file.contents + ); return flow( partial(setImportedFiles, importedFiles), - partial(transformContents, () => div.innerHTML) + partial(transformContents, () => contents) )(file); }; -export const composeHTML = cond([ - [ - testHTML, - flow( - partial(transformHeadTailAndContents, source => { - const div = document.createElement('div'); - div.innerHTML = source; - return div.innerHTML; - }), - partial(compileHeadTail, '') - ) - ], +const transformWithFrame = async function (transform, contents) { + // we use iframe here since file.contents is destined to be be inserted into + // the root of an iframe. + const frame = document.createElement('iframe'); + frame.style = 'display: none'; + let out = { contents }; + try { + // the frame needs to be inserted into the document to create the html + // element + document.body.appendChild(frame); + // replace the root element with user code + frame.contentDocument.documentElement.innerHTML = contents; + // grab the contents now, in case the transformation fails + out = { contents: frame.contentDocument.documentElement.innerHTML }; + out = await transform(frame); + } finally { + document.body.removeChild(frame); + } + return out; +}; + +const transformHtml = async function (file) { + const transform = async frame => { + await Promise.all([ + transformSASS(frame.contentDocument), + transformScript(frame.contentDocument) + ]); + return { contents: frame.contentDocument.documentElement.innerHTML }; + }; + + const { contents } = await transformWithFrame(transform, file.contents); + return transformContents(() => contents, file); +}; + +const composeHTML = cond([ + [testHTML, partial(compileHeadTail, '')], [stubTrue, identity] ]); -export const htmlTransformer = cond([ - [testHTML, flow(transformHtml, transformIncludes)], +const htmlTransformer = cond([ + [testHTML, flow(transformHtml, addImportedFiles)], [stubTrue, identity] ]); diff --git a/curriculum/challenges/english/14-responsive-web-design-22/learn-accessibility-by-building-a-quiz/step-002.md b/curriculum/challenges/english/14-responsive-web-design-22/learn-accessibility-by-building-a-quiz/step-002.md index 2d9fb6cd96..c27df411bc 100644 --- a/curriculum/challenges/english/14-responsive-web-design-22/learn-accessibility-by-building-a-quiz/step-002.md +++ b/curriculum/challenges/english/14-responsive-web-design-22/learn-accessibility-by-building-a-quiz/step-002.md @@ -19,13 +19,13 @@ You should create a `meta` element within the `head` element. ```js // TODO: Once builder is fixed so head info is not in body -assert.exists(document.querySelector('body > meta')); +assert.exists(document.querySelector('head > meta')); ``` You should give the `meta` tag a `charset` of `UTF-8`. ```js -assert.equal(document.querySelector('body > meta')?.getAttribute('charset'), 'UTF-8'); +assert.equal(document.querySelector('head > meta')?.getAttribute('charset'), 'UTF-8'); ``` # --seed-- diff --git a/curriculum/challenges/english/14-responsive-web-design-22/learn-accessibility-by-building-a-quiz/step-003.md b/curriculum/challenges/english/14-responsive-web-design-22/learn-accessibility-by-building-a-quiz/step-003.md index f22fb82bee..7abbdd8ce8 100644 --- a/curriculum/challenges/english/14-responsive-web-design-22/learn-accessibility-by-building-a-quiz/step-003.md +++ b/curriculum/challenges/english/14-responsive-web-design-22/learn-accessibility-by-building-a-quiz/step-003.md @@ -16,20 +16,20 @@ Add a `viewport` definition with a `content` attribute detailing the `width` and You should create another `meta` element in the `head`. ```js -assert.equal(document.querySelectorAll('body > meta')?.length, 2); +assert.equal(document.querySelectorAll('head > meta')?.length, 2); ``` You should give the `meta` a `name` attribute of `viewport`. ```js -assert.equal(document.querySelectorAll('body > meta[name="viewport"]')?.length, 1); +assert.equal(document.querySelectorAll('head > meta[name="viewport"]')?.length, 1); ``` You should give the `meta` a `content` attribute of `width=device-width, initial-scale=1`. ```js // TODO: Double-check this is the only correct answer -assert.equal(document.querySelectorAll('body > meta[content="width=device-width, initial-scale=1.0"]')?.length || document.querySelectorAll('body > meta[content="width=device-width, initial-scale=1"]')?.length, 1); +assert.equal(document.querySelectorAll('head > meta[content="width=device-width, initial-scale=1.0"]')?.length || document.querySelectorAll('body > meta[content="width=device-width, initial-scale=1"]')?.length, 1); ``` # --seed-- diff --git a/curriculum/challenges/english/14-responsive-web-design-22/learn-accessibility-by-building-a-quiz/step-005.md b/curriculum/challenges/english/14-responsive-web-design-22/learn-accessibility-by-building-a-quiz/step-005.md index c59274f4dd..a405fa5bf4 100644 --- a/curriculum/challenges/english/14-responsive-web-design-22/learn-accessibility-by-building-a-quiz/step-005.md +++ b/curriculum/challenges/english/14-responsive-web-design-22/learn-accessibility-by-building-a-quiz/step-005.md @@ -17,19 +17,19 @@ You should add a `title` element to the `head`. ```js // TODO: Fix once builder puts head in the right place -assert.exists(document.querySelector('body > title')); +assert.exists(document.querySelector('head > title')); ``` You should not make the `title` longer than 60 characters. ```js -assert.isAtMost(document.querySelector('body > title')?.textContent?.length, 60); +assert.isAtMost(document.querySelector('head > title')?.textContent?.length, 60); ``` Try being more descriptive with your `title` element. _Hint: At least 20 characters_ ```js -assert.isAtLeast(document.querySelector('body > title')?.textContent?.length, 20); +assert.isAtLeast(document.querySelector('head > title')?.textContent?.length, 20); ``` # --seed-- diff --git a/curriculum/package.json b/curriculum/package.json index 0b36b8e0d3..40463d1a3c 100644 --- a/curriculum/package.json +++ b/curriculum/package.json @@ -25,7 +25,7 @@ "delete-step": "cross-env CALLING_DIR=$INIT_CWD node ../tools/challenge-helper-scripts/delete-step", "lint": "gulp lint", "reorder-steps": "cross-env CALLING_DIR=$INIT_CWD node ../tools/challenge-helper-scripts/reorder-steps", - "test": "mocha --delay --exit --reporter progress --bail", + "test": "mocha --delay --exit --reporter progress --bail", "test:full-output": "cross-env FULL_OUTPUT=true mocha --delay --reporter progress" }, "devDependencies": { diff --git a/curriculum/test/test-challenges.js b/curriculum/test/test-challenges.js index 1a82768c47..b225bc01df 100644 --- a/curriculum/test/test-challenges.js +++ b/curriculum/test/test-challenges.js @@ -73,6 +73,14 @@ const { flatten, isEmpty, cloneDeep, isEqual } = lodash; // rethrow unhandled rejections to make sure the tests exit with -1 process.on('unhandledRejection', err => handleRejection(err)); +// If an uncaught exception gets here, then mocha is in an unexpected state. All +// we can do is log the exception and exit with a non-zero code. +process.on('uncaughtException', err => { + console.error('Uncaught exception:', err.message); + console.error(err.stack); + // eslint-disable-next-line no-process-exit + process.exit(1); +}); const handleRejection = err => { // setting the error code because node does not (yet) exit with a non-zero