Document that PhylogenyWrapper.getNodeLabels() and .getParsedNewick() may throw an Error

[gaurav]

PhylogenyWrapper.getNodeLabels() and PhylogenyWrapper.getParsedNewick() currently use the parse method in newick-js to parse Newick strings. See also PhylogenyWrapper.getParsedNewickWithIRIs(), which by default uses getParsedNewick() to parse the Newick.

phyx.js/src/wrappers/PhylogenyWrapper.js

Line 155 in d3623e3

const { graph } = parseNewick(this.phylogeny.newick || '()');

phyx.js/src/wrappers/PhylogenyWrapper.js

Line 255 in d3623e3

const { graph, root, rootWeight } = parseNewick(newick);

If the Newick string is malformed in particular ways (such as if they're empty), this will throw an Error, which at the moment we propagate to the code calling us. We should document this so that code that calls us knows to check for an exception.

One (slow) way of checking this is to run PhylogenyWrapper.getErrorsInNewickString(), which does wrap parse() -- so you can test whether a Newick string if valid for newick-js/phyx.js purposes by checking to see if this method returns an empty list before using either of the two methods described above. But that's probably unnecessarily complicated for most uses.

phyx.js/src/wrappers/PhylogenyWrapper.js

Line 78 in d3623e3

parseNewick(newickTrimmed);

[hlapp]

I agree, it sounds like this is what exceptions and try {} catch {} blocks are for.

[gaurav]

I was hoping I could close this just by adding a @throws to the documentation to let everybody know that this function might throw an exception, but in JavaScript -- unlike languages like Java -- there's no way to make sure that code calling this function traps any generated exception. This is particularly bad when running this code in a browser, where a thrown exception would stop the thread.

After thinking about it some more, I think a better solution might be to return a Promise -- that way, calling code could simply use .then(success) to silently ignore any errors, or .then(success).catch(error) if they can do something with the error. Since it's parseNewick() that actually throws the exception, we should create a single static method that parses Newick and returns a Promise; all functions that use parseNewick() would then use that method and return Promises themselves. This would guarantee that we never accidentally forget to handle Newick parsing errors, but at the cost of more complex code.

What do you think?

[hlapp]

in JavaScript -- unlike languages like Java -- there's no way to make sure that code calling this function traps any generated exception

Yes, but as you indicate this is a feature of the language. (Similarly, there is no type checking, and thus no way to somehow enforce that a calling client passes the correct types as arguments to a method. Arguably this, too, is a feature of the language, even if some believe it's a bad one.)

Point being, I don't know why we should feel obligated or even only inclined to try to somehow work around JavaScript's language feature where they differ from other languages.