improved error handling when composing in-shell

Fixes #805

Author: starpit

app/plugins/modules/composer/lib/create-from-source.js

@@ -14,6 +14,8 @@
  * limitations under the License.
  */
 
+const debug = require('debug')('composer compilation')
+
 const vm = require('vm'),
       fs = require('fs'),
       path = require('path'),
@@ -94,6 +96,7 @@ exports.compileToFSM = (src, opts={}) => new Promise((resolve, reject) => {
                     logMessage = ''
                     try {
                         const doExit = () => reject({
+                            statusCode: 'ENOPARSE',
                             fsm: errorMessage,
                             code: originalCode
                         })
@@ -169,25 +172,32 @@ ${errorMessage}`)
 
                         // for parse error, error message is shown in the fsm (JSON) tab, and user code in the source (code) tab
                         // reject now returns {fsm:errMsg, code:originalCode}
-                        reject(
-                            {
-                                fsm: message,
-                                code: originalCode
-                            }
-                        )
+                        return {
+                            statusCode: 'ENOPARSE',  // would like to use code here, but we've already used it for code:originalCode
+                            message,
+                            fsm: message,
+                            code: originalCode
+                        }
                     }
                 }
                 let fsm
                 try {
                     fsm = compile(originalCode)
                 } catch (err) {
+                    console.error('Catastrophic internal error compiling source')
                     console.error(err)
                 }
 
                 if (!isValidFSM(fsm)) {
                     // still no luck? reject
                     console.error('Error compiling app source', fsm, sandbox)
-                    reject('Your code could not be composed')
+                    if (fsm.statusCode) {
+                        debug('rejecting with pre-made error')
+                        reject(fsm)
+                    } else {
+                        reject('Your code could not be composed')
+                    }
+
                 } else {
                     if (opts.code) {
                         resolve({fsm, code: originalCode, localCodePath})

app/plugins/modules/composer/lib/create.js

@@ -33,7 +33,11 @@ debug('finished loading modules')
   */
 const handleFailure_fsmPromise = err => {
     if (err.fsm) {
-        throw new Error(err.fsm)
+        const error = new Error(err.fsm)
+        for (let key in err) {
+            error[key] = err[key]
+        }
+        throw error
     } else {
         throw err
     }

app/plugins/modules/editor/lib/edit.js

@@ -87,7 +87,7 @@ const persisters = {
     // persisters for regular actions
     actions: {
         getCode: action => action,
-        save: (wsk, action) => {
+        save: (wsk, action, editor) => {
             const owOpts = wsk.owOpts({
                 name: action.name,
                 namespace: action.namespace,
@@ -130,11 +130,11 @@ const persisters = {
                 }
             }
         }),
-        save: (wsk, app) => new Promise((resolve, reject) => {
+        save: (wsk, app, editor) => new Promise((resolve, reject) => {
             const fs = require('fs'),
                   tmp = require('tmp')
 
-            tmp.file({ prefix: 'shell-', postfix: '.js' }, (err, path, fd, cleanup) => {
+            tmp.file({ prefix: 'shell-', postfix: '.js' }, (err, filepath, fd, cleanup) => {
                 if (err) {
                     reject(err)
                 } else {
@@ -143,12 +143,65 @@ const persisters = {
                             reject(err)
                         } else {
                             // -r means try to deploy the actions, too
-                            return repl.qexec(`app update "${app.name}" "${path}" -r`)
+                            return repl.qexec(`app update "${app.name}" "${filepath}" -r`)
                                 .then(app => {
                                     cleanup()
-                                    console.error('#####', app)
                                     resolve(app)
                                 })
+                                .then(res => {
+                                    // successful compilation, so remove any parse error decorations
+                                    editor.clearDecorations()
+                                    return res
+                                })
+                                .catch(err => {
+                                    console.error('!!!!!!!', err)
+                                    if (err.statusCode === 'ENOPARSE') {
+                                        debug('composition did not parse', err)
+                                        const basename = path.basename(filepath),
+                                              pattern = new RegExp('\\n([^\n]+)\\n\\s+at\\s+' + basename.replace(/\./, '\\.') + ':(\\d+):(\\d+)'),
+                                              match = err.message.match(pattern)
+
+                                        debug('pattern', pattern)
+                                        debug('message', err.message)
+                                        if (match) {
+                                            const problem = match[1],
+                                                  line = match[2],
+                                                  column = match[3]
+                                            debug('got match', problem, line, column)
+
+                                            // see the 'hack it ourselves' just below
+                                            const rando = `shell-${new Date().getTime()}`
+
+                                            editor.__currentDecorations = editor.deltaDecorations(editor.__currentDecorations || [], [
+	                                        { range: new monaco.Range(line,1,line,1),
+                                                  options: { isWholeLine: true,
+                                                             //glyphMarginClassName: 'editor__parse-error-gutter-marker editor__parse_error_decoration',
+                                                             //glyphMarginHoverMessage: problem
+                                                             linesDecorationsClassName: `editor__parse-error-gutter-marker editor__parse-error-decoration ${rando}`
+                                                           }
+                                                },
+	                                        { range: new monaco.Range(line,column,line,column + 1),
+                                                  options: {
+                                                      beforeContentClassName: `editor__parse-error-before-marker editor__parse-error-decoration ${rando}`,
+                                                      //inlineClassName: 'editor__parse-error-inline-marker',
+                                                      hoverMessage: problem
+                                                  }
+                                                },
+                                            ])
+
+                                            // glyphMarginHoverMessage seems broken; hack it ourselves for now
+                                            setTimeout(() => {
+                                                const decos = document.querySelectorAll(`.${rando}`)
+                                                if (decos) {
+                                                    for (let idx = 0; idx < decos.length; idx++) {
+                                                        const deco = decos[idx]
+                                                        deco.setAttribute('title', problem)
+                                                    }
+                                                }
+                                            }, 0)
+                                        }
+                                    }
+                                })
                         }
                     })
                 }
@@ -177,7 +230,7 @@ const save = ({wsk, getAction, editor, eventBus}) => ({
         // https://github.com/apache/incubator-openwhisk/issues/3237
         delete action.version
 
-        return save(wsk, action)
+        return save(wsk, action, editor)
             .then(action => {
                 action.persister = persister
                 eventBus.emit('/editor/save', action, { event: 'save' })
@@ -385,6 +438,7 @@ const openEditor = wsk => {
                         minimap: {
 		            enabled: false
 	                },
+                        //glyphMargin: true,   // needed for error indicators
                         autoIndent: true,
                         codeLens: false,
                         quickSuggestions: false,
@@ -400,6 +454,11 @@ const openEditor = wsk => {
 	                language: 'javascript'
                     })
 
+                    editor.clearDecorations = () => {
+                        debug('clearing decorations')
+                        editor.__currentDecorations = editor.deltaDecorations(editor.__currentDecorations || [], [])
+                    }
+
                     resolve(editor)
                 })
             }
@@ -455,8 +514,11 @@ const openEditor = wsk => {
         modified.className = 'is-modified'
 
         // even handlers for saved and content-changed
-        const editsInProgress = () => sidecar.classList.add('is-modified')  // edits in progress
-        const editsCommitted = action => {                                  // edits committed
+        const editsInProgress = () => {
+            sidecar.classList.add('is-modified')
+            editor.clearDecorations() // for now, don't trty to be clever; remove decorations on any edit
+        }
+        const editsCommitted = action => {
             const lockIcon = sidecar.querySelector('[data-mode="lock"]')
 
             sidecar.classList.remove('is-modified')

app/plugins/modules/editor/lib/editor.css

@@ -101,3 +101,20 @@ body.sidecar-full-screen #sidecar .wskflow-container.visible {
     max-height: none;
     max-width: 50%;
 }
+
+
+/* gutter and inline decorations */
+.editor.parse-error-gutter-marker {
+    border-radius: 0.9375em;
+    background-color: var(--color-support-01);
+    width: 1ex !important;
+    margin-left: 1em;
+    /*background-image: repeating-linear-gradient(-45deg, transparent, transparent 10%, rgba(255,255,255,.5) 10%, rgba(255,255,255,.5) 15%);*/
+}
+.editor.parse-error-before-marker:before {
+    content: '^';
+    font-weight: bold;
+    position: absolute;
+    bottom: -1em;
+    color: var(--color-support-01);
+}

tests/tests/passes/07/composer-create-error-handling.js

@@ -95,7 +95,7 @@ SyntaxError: Unexpected token ,` }]
        .catch(common.oops(this)))
     dryRunBad.forEach( ({input, err}) => {
         it(`should dry-run check with expected error ${input} --dry-run`, () => cli.do(`app create ${input} --dry-run`, this.app)
-           .then(cli.expectError(0, err))
+           .then(cli.expectError('ENOPARSE', err))
            .catch(common.oops(this)))
     })
 

tests/tests/passes/07/composer-edit.js

@@ -81,4 +81,17 @@ describe('edit compositions', function() {
        .then(deploy(this.app, 'comp2'))
        .then(sidecar.expectBadge('v0.0.2'))
        .catch(common.oops(this)))
+
+    it(`should fail to open the editor for compose against existing composition`, () => cli.do('compose comp2', this.app)
+       .then(cli.expectError(409))
+       .catch(common.oops(this)))
+
+    it(`should open the editor to a new composition and expect error handling`, () => cli.do('compose comp3', this.app)
+       .then(cli.expectOK)
+       .then(sidecar.expectOpen)
+       .then(sidecar.expectShowing('comp3'))
+       .then(() => this.app.client.keys('composer.sequence(notfound1, notfound2)'))
+       .then(() => this.app.client.click(ui.selectors.SIDECAR_MODE_BUTTON('Deploy')))
+       .then(() => this.app.client.waitForExist('.editor.parse-error-decoration'))
+       .catch(common.oops(this)))
 })