chore(project): consistent async error handling

Async utilities properly propagate _all_ errors to the
specified _done_ callback. Failing in _done_ does not
re-invoke it.

Adjustments made in

* Viewer#importXML
* Viewer#saveSVG
* Importer#importBpmnDiagram

Tests cleaned up to remove un-necessary `if (err) { ...` snippets.

lib/Viewer.js

@@ -227,27 +227,33 @@ Viewer.prototype.saveSVG = function(options, done) {
     options = {};
   }
 
-  var canvas = this.get('canvas');
+  var svg, err;
 
-  var contentNode = canvas.getDefaultLayer(),
+  try {
-      defsNode = domQuery('defs', canvas._svg);
+    var canvas = this.get('canvas');
 
-  var contents = innerSVG(contentNode),
+    var contentNode = canvas.getDefaultLayer(),
-      defs = defsNode ? '<defs>' + innerSVG(defsNode) + '</defs>' : '';
+        defsNode = domQuery('defs', canvas._svg);
 
-  var bbox = contentNode.getBBox();
+    var contents = innerSVG(contentNode),
+        defs = defsNode ? '<defs>' + innerSVG(defsNode) + '</defs>' : '';
 
-  var svg =
+    var bbox = contentNode.getBBox();
-    '<?xml version="1.0" encoding="utf-8"?>\n' +
-    '<!-- created with bpmn-js / http://bpmn.io -->\n' +
-    '<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">\n' +
-    '<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" ' +
-         'width="' + bbox.width + '" height="' + bbox.height + '" ' +
-         'viewBox="' + bbox.x + ' ' + bbox.y + ' ' + bbox.width + ' ' + bbox.height + '" version="1.1">' +
-      defs + contents +
-    '</svg>';
 
-  done(null, svg);
+    svg =
+      '<?xml version="1.0" encoding="utf-8"?>\n' +
+      '<!-- created with bpmn-js / http://bpmn.io -->\n' +
+      '<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">\n' +
+      '<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" ' +
+           'width="' + bbox.width + '" height="' + bbox.height + '" ' +
+           'viewBox="' + bbox.x + ' ' + bbox.y + ' ' + bbox.width + ' ' + bbox.height + '" version="1.1">' +
+        defs + contents +
+      '</svg>';
+  } catch (e) {
+    err = e;
+  }
+
+  done(err, svg);
 };
 
 /**
@@ -292,12 +298,8 @@ Viewer.prototype.saveSVG = function(options, done) {
 
 Viewer.prototype.importDefinitions = function(definitions, done) {
 
-  var err;
+  // catch synchronous exceptions during #clear()
-
-  // use try/catch to not swallow synchronous exceptions
-  // that may be raised during model parsing
   try {
-
     if (this._definitions) {
       // clear existing rendered diagram
       this.clear();
@@ -305,15 +307,12 @@ Viewer.prototype.importDefinitions = function(definitions, done) {
 
     // update definitions
     this._definitions = definitions;
-
-    // perform graphical import
-    return importBpmnDiagram(this, definitions, done);
   } catch (e) {
-    err = e;
+    return done(e);
   }
 
-  // handle synchronous errors
+  // perform graphical import
-  return done(err);
+  return importBpmnDiagram(this, definitions, done);
 };
 
 Viewer.prototype.getModules = function() {

lib/import/Importer.js

@@ -14,9 +14,9 @@ var BpmnTreeWalker = require('./BpmnTreeWalker');
  */
 function importBpmnDiagram(diagram, definitions, done) {
 
-  var importer = diagram.get('bpmnImporter'),
+  var importer,
-      eventBus = diagram.get('eventBus'),
+      eventBus,
-      translate = diagram.get('translate');
+      translate;
 
   var error,
       warnings = [];
@@ -51,19 +51,23 @@ function importBpmnDiagram(diagram, definitions, done) {
     walker.handleDefinitions(definitions);
   }
 
-  eventBus.fire('import.render.start', { definitions: definitions });
-
   try {
+    importer = diagram.get('bpmnImporter');
+    eventBus = diagram.get('eventBus');
+    translate = diagram.get('translate');
+
+    eventBus.fire('import.render.start', { definitions: definitions });
+
     render(definitions);
+
+    eventBus.fire('import.render.complete', {
+      error: error,
+      warnings: warnings
+    });
   } catch (e) {
     error = e;
   }
 
-  eventBus.fire('import.render.complete', {
-    error: error,
-    warnings: warnings
-  });
-
   done(error, warnings);
 }
 

test/spec/ModelerSpec.js

@@ -72,18 +72,10 @@ describe('Modeler', function() {
     // given
     createModeler(xml, function(err, warnings, modeler) {
 
-      if (err) {
-        return done(err);
-      }
-
       // when
       // mimic re-import of same diagram
       modeler.importXML(xml, function(err, warnings) {
 
-        if (err) {
-          return done(err);
-        }
-
         // then
         expect(warnings.length).to.equal(0);
 

test/spec/ViewerSpec.js

@@ -54,18 +54,10 @@ describe('Viewer', function() {
     // given
     createViewer(xml, function(err, warnings, viewer) {
 
-      if (err) {
-        return done(err);
-      }
-
       // when
       // mimic re-import of same diagram
       viewer.importXML(xml, function(err, warnings) {
 
-        if (err) {
-          return done(err);
-        }
-
         // then
         expect(warnings.length).to.equal(0);
 
@@ -343,17 +335,9 @@ describe('Viewer', function() {
 
       createViewer(xml, function(err, warnings, viewer) {
 
-        if (err) {
-          return done(err);
-        }
-
         // when
         viewer.saveXML({ format: true }, function(err, xml) {
 
-          if (err) {
-            return done(err);
-          }
-
           // then
           expect(xml).to.contain('<?xml version="1.0" encoding="UTF-8"?>');
           expect(xml).to.contain('<bpmn2:definitions');
@@ -380,14 +364,10 @@ describe('Viewer', function() {
         // when
         viewer.saveSVG(function(err, svg) {
 
-          if (err) {
-            return done(err);
-          }
-
           // then
           expect(validSVG(svg)).to.be.true;
 
-          done();
+          done(err);
         });
       });
     });
@@ -411,17 +391,13 @@ describe('Viewer', function() {
         // when
         viewer.saveSVG(function(err, svg) {
 
-          if (err) {
-            return done(err);
-          }
-
           // then
           expect(validSVG(svg)).to.be.true;
 
           // no svg export should not take too long
           expect(currentTime() - time).to.be.below(1000);
 
-          done();
+          done(err);
         });
       });
     });
@@ -459,10 +435,6 @@ describe('Viewer', function() {
         // when
         viewer.saveSVG(function(err, svg) {
 
-          if (err) {
-            return done(err);
-          }
-
           var svgDoc = document.createElementNS('http://www.w3.org/2000/svg', 'svg');
           svgDoc.innerHTML = svg;
 
@@ -470,7 +442,7 @@ describe('Viewer', function() {
           expect(validSVG(svg)).to.be.true;
           expect(svgDoc.querySelector('.outer-bound-marker')).to.be.null;
 
-          done();
+          done(err);
         });
       });
     });

test/spec/draw/BpmnRendererSpec.js

@@ -180,10 +180,6 @@ describe('draw - bpmn renderer', function() {
 
     bootstrapViewer(xml)(function(err) {
 
-      if (err) {
-        return done(err);
-      }
-
       inject(function(elementRegistry) {
 
         var callActivityGfx = elementRegistry.getGraphics('CallActivity');
@@ -191,7 +187,7 @@ describe('draw - bpmn renderer', function() {
         // make sure the + marker is shown
         expect(domQuery('[data-marker=sub-process]', callActivityGfx)).to.exist;
 
-        done();
+        done(err);
       })();
     });
 
@@ -203,10 +199,6 @@ describe('draw - bpmn renderer', function() {
 
     bootstrapViewer(xml)(function(err) {
 
-      if (err) {
-        return done(err);
-      }
-
       inject(function(elementRegistry) {
 
         var callActivityGfx = elementRegistry.getGraphics('AdHocSubProcess');
@@ -214,7 +206,7 @@ describe('draw - bpmn renderer', function() {
         // make sure the + marker is shown
         expect(domQuery('[data-marker=adhoc]', callActivityGfx)).to.exist;
 
-        done();
+        done(err);
       })();
     });
 
@@ -226,10 +218,6 @@ describe('draw - bpmn renderer', function() {
     var xml = require('../../fixtures/bpmn/simple.bpmn');
     bootstrapViewer(xml)(function(err) {
 
-      if (err) {
-        return done(err);
-      }
-
       inject(function(canvas) {
         var svg = canvas._svg;
         var markers = svg.querySelectorAll('marker');
@@ -237,7 +225,7 @@ describe('draw - bpmn renderer', function() {
         expect(markers[0].id).to.match(/^sequenceflow-end-white-black-[A-Za-z0-9]+$/);
       })();
 
-      done();
+      done(err);
     });
   });
 
@@ -247,10 +235,6 @@ describe('draw - bpmn renderer', function() {
     var xml = require('../../fixtures/bpmn/draw/colors.bpmn');
     bootstrapViewer(xml)(function(err) {
 
-      if (err) {
-        return done(err);
-      }
-
       inject(function(canvas) {
         var svg = canvas._svg;
         var markers = svg.querySelectorAll('marker');
@@ -260,7 +244,7 @@ describe('draw - bpmn renderer', function() {
         expect(markers[4].id).to.match(/^messageflow-start-white-fuchsia-[A-Za-z0-9]{25}$/);
       })();
 
-      done();
+      done(err);
     });
   });
 

test/spec/features/modeling/LabelBoundsSpec.js

@@ -246,10 +246,6 @@ describe('label bounds', function() {
 
         modeler.saveXML({ format: true }, function(err, result) {
 
-          if (err) {
-            return done(err);
-          }
-
           // strip spaces and line breaks after '>'
           result = result.replace(/>\s+/g,'>');
 
@@ -262,7 +258,7 @@ describe('label bounds', function() {
           expect(width).to.equal(shape.label.width);
           expect(height).to.equal(shape.label.height);
 
-          done();
+          done(err);
         });
       });
     });
@@ -289,10 +285,6 @@ describe('label bounds', function() {
 
         modeler.saveXML({ format: true }, function(err, result) {
 
-          if (err) {
-            return done(err);
-          }
-
           // strip spaces and line breaks after '>'
           result = result.replace(/>\s+/g,'>');
 
@@ -305,7 +297,7 @@ describe('label bounds', function() {
           expect(width).to.equal(shape.label.width);
           expect(height).to.equal(shape.label.height);
 
-          done();
+          done(err);
         });
       });
     });
@@ -332,7 +324,7 @@ describe('label bounds', function() {
 
           expect(result).to.equal(xml);
 
-          done();
+          done(err);
         });
       });
     });

test/spec/import/ImporterSpec.js

@@ -7,7 +7,7 @@ var TestContainer = require('mocha-test-container-support');
 
 var Diagram = require('diagram-js/lib/Diagram'),
     BpmnModdle = require('bpmn-moddle'),
-    Importer = require('../../../lib/import/Importer'),
+    importBpmnDiagram = require('../../../lib/import/Importer').importBpmnDiagram,
     Viewer = require('../../../lib/Viewer');
 
 var find = require('lodash/collection/find');
@@ -34,11 +34,7 @@ describe('import - Importer', function() {
     var moddle = new BpmnModdle();
 
     moddle.fromXML(xml, function(err, definitions) {
-      if (err) {
+      importBpmnDiagram(diagram, definitions, done);
-        return done(err);
-      }
-
-      Importer.importBpmnDiagram(diagram, definitions, done);
     });
   }
 

test/spec/import/elements/AssociationSpec.js

@@ -16,10 +16,6 @@ describe('import - associations', function() {
       // given
       bootstrapViewer(xml)(function(err) {
 
-        if (err) {
-          return done(err);
-        }
-
         // when
         inject(function(elementRegistry) {
 
@@ -28,7 +24,7 @@ describe('import - associations', function() {
           // then
           expect(association).to.exist;
 
-          done();
+          done(err);
         })();
 
       });
@@ -42,10 +38,6 @@ describe('import - associations', function() {
       // given
       bootstrapViewer(xml)(function(err) {
 
-        if (err) {
-          return done(err);
-        }
-
         // when
         inject(function(elementRegistry) {
 
@@ -90,17 +82,13 @@ describe('import - associations', function() {
       // given
       bootstrapViewer(xml)(function(err) {
 
-        if (err) {
-          return done(err);
-        }
-
         // then
         expectRendered([
           'DataInputAssociation',
           'DataOutputAssociation'
         ]);
 
-        done();
+        done(err);
       });
     });
 
@@ -112,17 +100,13 @@ describe('import - associations', function() {
       // given
       bootstrapViewer(xml)(function(err) {
 
-        if (err) {
-          return done(err);
-        }
-
         // then
         expectRendered([
           'DataInputAssociation',
           'DataOutputAssociation'
         ]);
 
-        done();
+        done(err);
       });
     });
 
@@ -134,18 +118,13 @@ describe('import - associations', function() {
       // given
       bootstrapViewer(xml)(function(err) {
 
-        if (err) {
-          return done(err);
-        }
-
         // then
         expectRendered([
           'DataInputAssociation',
           'DataOutputAssociation'
         ]);
 
-        done();
+        done(err);
-
       });
     });
 
@@ -157,17 +136,13 @@ describe('import - associations', function() {
       // given
       bootstrapViewer(xml)(function(err) {
 
-        if (err) {
-          return done(err);
-        }
-
         // then
         expectRendered([
           'DataInputAssociation',
           'DataOutputAssociation'
         ]);
 
-        done();
+        done(err);
       });
     });
 

test/spec/import/elements/LabelSpec.js

@@ -44,10 +44,6 @@ describe('import - labels', function() {
       // given
       bootstrapViewer(xml)(function(err) {
 
-        if (err) {
-          return done(err);
-        }
-
         // when
         inject(function(elementRegistry) {
 
@@ -68,7 +64,7 @@ describe('import - labels', function() {
           expect(sequenceFlowLabel.width).to.be.above(64);
           expect(sequenceFlowLabel.height).to.be.above(11);
 
-          done();
+          done(err);
         })();
 
       });