From c5ffa11a235f990af4c89692f23a0d71f9310dff Mon Sep 17 00:00:00 2001 From: Jacob Pozaic Date: Mon, 11 Oct 2021 10:59:49 -0400 Subject: [PATCH] Improved Alpine error resiliency and logging (#2027) * Iteration on error logging, adding event on errors for plugins to listen to. * Iteration on error logging, adding event on errors for plugins to listen to. ( pulled package-lock.json ) * Iteration on error logging, adding event on errors for plugins to listen to. -> fixed import from csp * move el and expression onto detail object directly * add back error for better dev experience * fix two bad tests that fail when exception handling is working * remove test since empty x-init is not valid * remove error event dispatching * remove unnecessary try/catch that should be handled internally * Fixed extra tab * Removed unused import * prevent useless 'func is not a function' errors Co-authored-by: jacobp Co-authored-by: dand --- packages/alpinejs/src/directives/x-data.js | 4 + packages/alpinejs/src/evaluator.js | 57 ++--- packages/alpinejs/src/utils/error.js | 15 ++ packages/csp/src/index.js | 3 +- .../integration/custom-directives.spec.js | 2 - .../integration/error-handling.spec.js | 202 ++++++++++++++++++ tests/cypress/integration/store.spec.js | 2 +- tests/cypress/utils.js | 28 ++- 8 files changed, 272 insertions(+), 41 deletions(-) create mode 100644 packages/alpinejs/src/utils/error.js create mode 100644 tests/cypress/integration/error-handling.spec.js diff --git a/packages/alpinejs/src/directives/x-data.js b/packages/alpinejs/src/directives/x-data.js index bb76b7b71..81ad20a2e 100644 --- a/packages/alpinejs/src/directives/x-data.js +++ b/packages/alpinejs/src/directives/x-data.js @@ -21,6 +21,10 @@ directive('data', skipDuringClone((el, { expression }, { cleanup }) => { let data = evaluate(el, expression, { scope: dataProviderContext }) + if( data === undefined ) { + data = {} + } + injectMagics(data, el) let reactiveData = reactive(data) diff --git a/packages/alpinejs/src/evaluator.js b/packages/alpinejs/src/evaluator.js index 866599fa9..8a745f386 100644 --- a/packages/alpinejs/src/evaluator.js +++ b/packages/alpinejs/src/evaluator.js @@ -1,5 +1,6 @@ import { closestDataStack, mergeProxies } from './scope' import { injectMagics } from './magics' +import { tryCatch, handleError } from './utils/error' export function evaluate(el, expression, extras = {}) { let result @@ -30,7 +31,7 @@ export function normalEvaluator(el, expression) { return generateEvaluatorFromFunction(dataStack, expression) } - let evaluator = generateEvaluatorFromString(dataStack, expression) + let evaluator = generateEvaluatorFromString(dataStack, expression, el) return tryCatch.bind(null, el, expression, evaluator) } @@ -45,7 +46,7 @@ export function generateEvaluatorFromFunction(dataStack, func) { let evaluatorMemo = {} -function generateFunctionFromString(expression) { +function generateFunctionFromString(expression, el) { if (evaluatorMemo[expression]) { return evaluatorMemo[expression] } @@ -63,15 +64,23 @@ function generateFunctionFromString(expression) { ? `(() => { ${expression} })()` : expression - let func = new AsyncFunction(['__self', 'scope'], `with (scope) { __self.result = ${rightSideSafeExpression} }; __self.finished = true; return __self.result;`) + const safeAsyncFunction = () => { + try { + return new AsyncFunction(['__self', 'scope'], `with (scope) { __self.result = ${rightSideSafeExpression} }; __self.finished = true; return __self.result;`) + } catch ( error ) { + handleError( error, el, expression ) + return Promise.resolve() + } + } + let func = safeAsyncFunction() evaluatorMemo[expression] = func return func } -function generateEvaluatorFromString(dataStack, expression) { - let func = generateFunctionFromString(expression) +function generateEvaluatorFromString(dataStack, expression, el) { + let func = generateFunctionFromString(expression, el) return (receiver = () => {}, { scope = {}, params = [] } = {}) => { func.result = undefined @@ -81,27 +90,29 @@ function generateEvaluatorFromString(dataStack, expression) { let completeScope = mergeProxies([ scope, ...dataStack ]) - let promise = func(func, completeScope) - - // Check if the function ran synchronously, - if (func.finished) { - // Return the immediate result. - runIfTypeOfFunction(receiver, func.result, completeScope, params) - } else { - // If not, return the result when the promise resolves. - promise.then(result => { - runIfTypeOfFunction(receiver, result, completeScope, params) - }) + if( typeof func === 'function' ) { + let promise = func(func, completeScope).catch((error) => handleError(error, el, expression)) + + // Check if the function ran synchronously, + if (func.finished) { + // Return the immediate result. + runIfTypeOfFunction(receiver, func.result, completeScope, params, el) + } else { + // If not, return the result when the promise resolves. + promise.then(result => { + runIfTypeOfFunction(receiver, result, completeScope, params, el) + }).catch( error => handleError( error, el, expression ) ) + } } } } -export function runIfTypeOfFunction(receiver, value, scope, params) { +export function runIfTypeOfFunction(receiver, value, scope, params, el) { if (typeof value === 'function') { let result = value.apply(scope, params) if (result instanceof Promise) { - result.then(i => runIfTypeOfFunction(receiver, i, scope, params)) + result.then(i => runIfTypeOfFunction(receiver, i, scope, params)).catch( error => handleError( error, el, value ) ) } else { receiver(result) } @@ -109,13 +120,3 @@ export function runIfTypeOfFunction(receiver, value, scope, params) { receiver(value) } } - -export function tryCatch(el, expression, callback, ...args) { - try { - return callback(...args) - } catch (e) { - console.warn(`Alpine Expression Error: ${e.message}\n\nExpression: "${expression}"\n\n`, el) - - throw e - } -} diff --git a/packages/alpinejs/src/utils/error.js b/packages/alpinejs/src/utils/error.js new file mode 100644 index 000000000..62a202836 --- /dev/null +++ b/packages/alpinejs/src/utils/error.js @@ -0,0 +1,15 @@ +export function tryCatch(el, expression, callback, ...args) { + try { + return callback(...args) + } catch (e) { + handleError( e, el, expression ) + } +} + +export function handleError(error, el, expression = undefined) { + Object.assign( error, { el, expression } ) + + console.warn(`Alpine Expression Error: ${error.message}\n\n${ expression ? 'Expression: \"' + expression + '\"\n\n' : '' }`, el) + + setTimeout( () => { throw error }, 0 ) +} diff --git a/packages/csp/src/index.js b/packages/csp/src/index.js index 2f69a10b7..9b5b26c63 100644 --- a/packages/csp/src/index.js +++ b/packages/csp/src/index.js @@ -10,7 +10,8 @@ import 'alpinejs/src/directives/index' import { closestDataStack, mergeProxies } from 'alpinejs/src/scope' import { injectMagics } from 'alpinejs/src/magics' -import { generateEvaluatorFromFunction, runIfTypeOfFunction, tryCatch } from 'alpinejs/src/evaluator' +import { generateEvaluatorFromFunction, runIfTypeOfFunction } from 'alpinejs/src/evaluator' +import { tryCatch } from 'alpinejs/src/utils/error' function cspCompliantEvaluator(el, expression) { let overriddenMagics = {} diff --git a/tests/cypress/integration/custom-directives.spec.js b/tests/cypress/integration/custom-directives.spec.js index ac9ef05e0..41aa209f6 100644 --- a/tests/cypress/integration/custom-directives.spec.js +++ b/tests/cypress/integration/custom-directives.spec.js @@ -26,7 +26,6 @@ test('directives are auto cleaned up', `, ` Alpine.directive('foo', (el, {}, { effect, cleanup, evaluateLater }) => { - let evaluate = evaluateLater('foo') let incCount = evaluateLater('count++') cleanup(() => { @@ -36,7 +35,6 @@ test('directives are auto cleaned up', effect(() => { incCount() - evaluate(value => el.textContent = value) }) }) `], diff --git a/tests/cypress/integration/error-handling.spec.js b/tests/cypress/integration/error-handling.spec.js new file mode 100644 index 000000000..9e8360143 --- /dev/null +++ b/tests/cypress/integration/error-handling.spec.js @@ -0,0 +1,202 @@ +import { haveText, html, test } from '../utils' + +export function setupConsoleInterceptor( ...targetIds ) { + const mappedTargetIds = targetIds.map( tid => `'${tid}'` ).join( ',' ) + return ` + let errorContainer = document.createElement('div'); + errorContainer.id = 'errors' + errorContainer.textContent = 'false' + document.querySelector('#root').after(errorContainer) + console.warnlog = console.warn.bind(console) + console.warn = function () { + document.getElementById( 'errors' ).textContent = [${mappedTargetIds}].some( target => arguments[1] === document.getElementById( target ) ) + console.warnlog.apply(console, arguments) + } + ` +} + +export function assertConsoleInterceptorHadErrorWithCorrectElement() { + return ({get}) => { + get('#errors').should(haveText('true')) + }; +} + +test('x-for identifier issue', + [html` +
+ +
+ `, + setupConsoleInterceptor( "xfor" ) + ], + assertConsoleInterceptorHadErrorWithCorrectElement(), + true +) + +test('x-text identifier issue', + [html` +
+ +
+ `, + setupConsoleInterceptor( "xtext" ) + ], + assertConsoleInterceptorHadErrorWithCorrectElement(), + true +) + +test('x-init identifier issue', + [html` +
+
+ `, + setupConsoleInterceptor( "xinit" ) + ], + assertConsoleInterceptorHadErrorWithCorrectElement(), + true +) + +test('x-show identifier issue', + [html` +
+
+ `, + setupConsoleInterceptor( "xshow" ) + ], + assertConsoleInterceptorHadErrorWithCorrectElement(), + true +) + +test('x-bind class object syntax identifier issue', + [html` +
+
+
+ `, + setupConsoleInterceptor( "xbind" ) + ], + assertConsoleInterceptorHadErrorWithCorrectElement(), + true +) + +test('x-model identifier issue', + [html` +
+ +
+ `, + setupConsoleInterceptor( "xmodel" ) + ], + assertConsoleInterceptorHadErrorWithCorrectElement(), + true +) + +test('x-if identifier issue', + [html` +
+ +
+ `, + setupConsoleInterceptor( "xif" ) + ], + assertConsoleInterceptorHadErrorWithCorrectElement(), + true +) + +test('x-if identifier issue ( function )', + [html` +
+ +
+ `, + setupConsoleInterceptor( "xif" ) + ], + assertConsoleInterceptorHadErrorWithCorrectElement(), + true +) + +test('x-effect identifier issue', + [html` +
+
+ `, + setupConsoleInterceptor( "xeffect" ) + ], + assertConsoleInterceptorHadErrorWithCorrectElement(), + true +) + +test('x-on identifier issue', + [html` +
+
+ +
+ `, + setupConsoleInterceptor( "xon" ) + ], + ({ get }) => { + get( "#xon" ).click() + get( "#errors" ).should(haveText('true')) + }, + true +) + +test('x-data syntax error', + [html` +
+
+ `, + setupConsoleInterceptor( "xdata" ) + ], + assertConsoleInterceptorHadErrorWithCorrectElement(), + true +) + +test('if statement syntax error', + [html` +
+
+
+ `, + setupConsoleInterceptor( "xtext" ) + ], + assertConsoleInterceptorHadErrorWithCorrectElement(), + true +) + +test('x-data with reference error and multiple errors', + [html` +
+ +
+ `, + setupConsoleInterceptor( "xdata", "xtext" ) + ], + assertConsoleInterceptorHadErrorWithCorrectElement(), + true +) + +test('evaluation with syntax error', + [html` +
+ +
+ `, + setupConsoleInterceptor( "xif" ) + ], + assertConsoleInterceptorHadErrorWithCorrectElement(), + true +) diff --git a/tests/cypress/integration/store.spec.js b/tests/cypress/integration/store.spec.js index 6521f36b3..bd7dc992b 100644 --- a/tests/cypress/integration/store.spec.js +++ b/tests/cypress/integration/store.spec.js @@ -70,7 +70,7 @@ test('store\'s "this" context is reactive for init function', [html`
- +
`, ` diff --git a/tests/cypress/utils.js b/tests/cypress/utils.js index 927f8bdcd..a8033d61b 100644 --- a/tests/cypress/utils.js +++ b/tests/cypress/utils.js @@ -5,19 +5,19 @@ export function html(strings) { return strings.raw[0] } -export let test = function (name, template, callback) { +export let test = function (name, template, callback, handleExpectedErrors = false) { it(name, () => { - injectHtmlAndBootAlpine(cy, template, callback) + injectHtmlAndBootAlpine(cy, template, callback, undefined, handleExpectedErrors) }) } -test.only = (name, template, callback) => { +test.only = (name, template, callback, handleExpectedErrors = false) => { it.only(name, () => { - injectHtmlAndBootAlpine(cy, template, callback) + injectHtmlAndBootAlpine(cy, template, callback, undefined, handleExpectedErrors) }) } -test.retry = (count) => (name, template, callback) => { +test.retry = (count) => (name, template, callback, handleExpectedErrors = false) => { it(name, { retries: { // During "cypress run" @@ -26,23 +26,33 @@ test.retry = (count) => (name, template, callback) => { openMode: count - 1, } }, () => { - injectHtmlAndBootAlpine(cy, template, callback) + injectHtmlAndBootAlpine(cy, template, callback, undefined, handleExpectedErrors) }) } -test.csp = (name, template, callback) => { +test.csp = (name, template, callback, handleExpectedErrors = false) => { it(name, () => { - injectHtmlAndBootAlpine(cy, template, callback, __dirname+'/spec-csp.html') + injectHtmlAndBootAlpine(cy, template, callback, __dirname+'/spec-csp.html', handleExpectedErrors) }) } -function injectHtmlAndBootAlpine(cy, templateAndPotentiallyScripts, callback, page) { +function injectHtmlAndBootAlpine(cy, templateAndPotentiallyScripts, callback, page, handleExpectedErrors = false) { let [template, scripts] = Array.isArray(templateAndPotentiallyScripts) ? templateAndPotentiallyScripts : [templateAndPotentiallyScripts] cy.visit(page || __dirname+'/spec.html') + if( handleExpectedErrors ) { + cy.on( 'uncaught:exception', ( error ) => { + if( error.el === undefined && error.expression === undefined ) { + console.warn( 'Expected all errors originating from Alpine to have el and expression. Letting cypress fail the test.', error ) + return true + } + return false + } ); + } + cy.get('#root').then(([el]) => { el.innerHTML = template