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 <mozillafirefox1> Co-authored-by: dand <daniel.dent@borderconnect.com>
alpinejs · Oct 11, 2021 · c5ffa11 · c5ffa11
1 parent 52ff275
commit c5ffa11
Show file tree

Hide file tree

Showing 8 changed files with 272 additions and 41 deletions.
diff --git 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
@@ -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,41 +90,33 @@ 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)
         }
     } else {
         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
@@ -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
@@ -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
@@ -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
@@ -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`
+        <div x-data="{ items: ['foo'] }">
+            <template id="xfor" x-for="item in itemzzzz">
+                <span x-text="item"></span>
+            </template>
+        </div>
+    `,
+        setupConsoleInterceptor( "xfor" )
+    ],
+    assertConsoleInterceptorHadErrorWithCorrectElement(),
+    true
+)
+
+test('x-text identifier issue',
+    [html`
+        <div x-data="{ items: ['foo'] }">
+            <template x-for="item in items">
+                <span id="xtext" x-text="itemzzz"></span>
+            </template>
+        </div>
+    `,
+        setupConsoleInterceptor( "xtext" )
+    ],
+    assertConsoleInterceptorHadErrorWithCorrectElement(),
+    true
+)
+
+test('x-init identifier issue',
+    [html`
+        <div id="xinit" x-data x-init="doesNotExist()">
+        </div>
+    `,
+        setupConsoleInterceptor( "xinit" )
+    ],
+    assertConsoleInterceptorHadErrorWithCorrectElement(),
+    true
+)
+
+test('x-show identifier issue',
+    [html`
+        <div id="xshow" x-data="{isOpen: true}" x-show="isVisible">
+        </div>
+    `,
+        setupConsoleInterceptor( "xshow" )
+    ],
+    assertConsoleInterceptorHadErrorWithCorrectElement(),
+    true
+)
+
+test('x-bind class object syntax identifier issue',
+    [html`
+        <div x-data="{isOpen: true}">
+            <div id="xbind" :class="{ 'block' : isVisible, 'hidden' : !isVisible }"></div>
+        </div>
+    `,
+        setupConsoleInterceptor( "xbind" )
+    ],
+    assertConsoleInterceptorHadErrorWithCorrectElement(),
+    true
+)
+
+test('x-model identifier issue',
+    [html`
+        <div x-data="{value: ''}">
+            <input id="xmodel" x-model="thething"/>
+        </div>
+    `,
+        setupConsoleInterceptor( "xmodel" )
+    ],
+    assertConsoleInterceptorHadErrorWithCorrectElement(),
+    true
+)
+
+test('x-if identifier issue',
+    [html`
+        <div x-data="{value: ''}">
+            <template id="xif" x-if="valuez === ''">
+                <span>Words</span>
+            </template>
+        </div>
+    `,
+        setupConsoleInterceptor( "xif" )
+    ],
+    assertConsoleInterceptorHadErrorWithCorrectElement(),
+    true
+)
+
+test('x-if identifier issue ( function )',
+    [html`
+        <div x-data="{shouldOpen: function(){}}">
+            <template id="xif" x-if="isOpen()">
+                <span>Words</span>
+            </template>
+        </div>
+    `,
+        setupConsoleInterceptor( "xif" )
+    ],
+    assertConsoleInterceptorHadErrorWithCorrectElement(),
+    true
+)
+
+test('x-effect identifier issue',
+    [html`
+        <div id="xeffect" x-data="{ label: 'Hello' }" x-effect="System.out.println(label)">
+        </div>
+    `,
+        setupConsoleInterceptor( "xeffect" )
+    ],
+    assertConsoleInterceptorHadErrorWithCorrectElement(),
+    true
+)
+
+test('x-on identifier issue',
+    [html`
+        <div x-data="{ label: 'Hello' }">
+            <div x-text="label"></div>
+            <button id="xon" x-on:click="labelz += ' World!'">Change Message</button>
+        </div>
+    `,
+        setupConsoleInterceptor( "xon" )
+    ],
+    ({ get }) => {
+        get( "#xon" ).click()
+        get( "#errors" ).should(haveText('true'))
+    },
+    true
+)
+
+test('x-data syntax error',
+    [html`
+        <div id="xdata" x-data="{ label: 'Hello' }aaa">
+        </div>
+    `,
+        setupConsoleInterceptor( "xdata" )
+    ],
+    assertConsoleInterceptorHadErrorWithCorrectElement(),
+    true
+)
+
+test('if statement syntax error',
+    [html`
+        <div x-data="{ label: 'Hello' }">
+            <div id="xtext" x-text="if( false { label } else { 'bye' }"></div>
+        </div>
+    `,
+        setupConsoleInterceptor( "xtext" )
+    ],
+    assertConsoleInterceptorHadErrorWithCorrectElement(),
+    true
+)
+
+test('x-data with reference error and multiple errors',
+    [html`
+        <div id="xdata" x-data="{ items : [ {v:'one'},{v:'two'}], replaceItems }">
+            <template id="xtext" x-for="item in items">
+                <span x-text="item.v"></span>
+            </template>
+        </div>
+    `,
+        setupConsoleInterceptor( "xdata", "xtext" )
+    ],
+    assertConsoleInterceptorHadErrorWithCorrectElement(),
+    true
+)
+
+test('evaluation with syntax error',
+    [html`
+        <div x-data="{value: ''}">
+            <template id="xif" x-if="value ==== ''">
+                <span>Words</span>
+            </template>
+        </div>
+    `,
+        setupConsoleInterceptor( "xif" )
+    ],
+    assertConsoleInterceptorHadErrorWithCorrectElement(),
+    true
+)
diff --git 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`
         <div x-data>
         <span x-text="$store.test.count"></span>
-        <button @click="$store.test.increment()" id="button">increment</button>
+        <button id="button">increment</button>
         </div>
     `,
     `