workbook.write throws instead of calling callback

[AurelienRibon]

Function workbook.write ends with these lines:

excel4node/source/lib/workbook/workbook.js

Lines 194 to 196 in 1c4a7bf

	.catch((e) => {
	throw new Error(e.stack);
	});

Since we are inside a promise rejection, it means this error thrown will never be caught by anyone, which just crashes nodejs with an UnhandledPromiseRejectionWarning.

The solution is to call the callback if it was provided, or just log the error and return gracefully if there is no callback.

.catch((e) => { 
  if (typeof handler === 'function') {
    return handler(e);
  }
  
  console.error(e.stack);  
}); 

For the record, the error we got in production is:

(node:30664) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 2): Error: Error: Invalid character (�) in string: Would be 🏼 at index 9
    at XMLStringifier.module.exports.XMLStringifier.assertLegalChar (/apps/core.stoic/node_modules/xmlbuilder/lib/XMLStringifier.js:153:15)
    at XMLStringifier.assertLegalChar (/apps/core.stoic/node_modules/xmlbuilder/lib/XMLStringifier.js:4:59)
    at XMLStringifier.module.exports.XMLStringifier.eleText (/apps/core.stoic/node_modules/xmlbuilder/lib/XMLStringifier.js:33:19)
    at new XMLText (/apps/core.stoic/node_modules/xmlbuilder/lib/XMLText.js:17:35)
    at XMLElement.module.exports.XMLNode.text (/apps/core.stoic/node_modules/xmlbuilder/lib/XMLNode.js:165:15)
    at XMLElement.module.exports.XMLNode.txt (/apps/core.stoic/node_modules/xmlbuilder/lib/XMLNode.js:365:19)
    at promiseObj.wb.sharedStrings.forEach (/apps/core.stoic/node_modules/excel4node/source/lib/workbook/builder.js:236:40)
    at Array.forEach (<anonymous>)
    at Promise (/apps/core.stoic/node_modules/excel4node/source/lib/workbook/builder.js:234:37)
    at Promise (<anonymous>)

[natergj]

I'm planning on re-working the whole error handling in the next major version which I'm working on this winter. As a temporary solution, you could wrap the workbook.write() call in a try/catch block and log out the stack that is thrown. I'd rather not simply call console.error() in the module. I want to make sure that anyone who uses the module has the ability to be informed when an error occurs so that they can properly handle it. In this instance, the catch block in your code could also include some logic to give some sort of feedback to the user that the excel workbook generation failed.
Calling the callback with the error if the callback was provided is a good option. That would be a breaking change in case someone is currently using try/catch to handle workbook.write() failures and that change shouldn't go into a minor or patch version update.

[AurelienRibon]

This is not as simple, users cannot workaround the current issue by using a try/catch around workbook.write. The throw new Error(err.stack) you do at line 195 is done inside a promise catch clause. When this clause is called, you are outside the initial event loop that was used to call workbook.write, so the try/catch was already exited, the error is really lost and will crash the server.

A simple reproduction case is the following one. Try it and you'll see that you never enter the catch clause of the try/catch block. That's why at least calling the callback with the error seems mandatory :)

const promise = new Promise(() => setTimeout(() => BOOM(), 1000));

try {
  promise.catch(err => { throw err });
} catch (err) {
  // This is never reached!
  console.log('Hey, I caught the error! ' + err.message);
}

[natergj]

oh, right. I'll change this to a bug and get a fix put into the next patch release

[AurelienRibon]

Awesome, thanks a lot !

[natergj]

Fix has now been pushed to NPM with version 1.3.1