call callback with error instead of throwing #12
Conversation
|
|
||
| if (faces.length > 1) throw new Error('woah multifont!'); | ||
| if (faces.length > 1) { | ||
| return new Error('Multiple fonts are not yet supported.'); |
There was a problem hiding this comment.
Should be return callback(new Error...)) no?
|
oooooops
The reason there wasn't a test for it was because we didn't know how it was triggered — the FreeType spec says certain fonts can have multiple faces in one file, but this is rare. @miccolis did the log reveal which font crashed it? |
|
@lbud nope, in typical crashy crash fashion, it just crashed and all I got was this lousy stack trace... |
|
Okay, next commit includes Noto, hits decent coverage numbers, switches to TAP. Unfortunately node-fontnik also has its own throw, so I'm wrapping the call in a try/catch now. Upstream for that is mapbox/node-fontnik#112 |
| @@ -29,7 +31,31 @@ tape('handle undefined style_name', function(t) { | |||
| }); | |||
| }); | |||
|
|
|||
| tape('font machine 256-511', function(t) { | |||
| test('handle font not found', function(t) { | |||
| fontmachine.makeGlyphs({font: 'NOT-FOUND', filetype: '.ttc'}, function(err, font) { | |||
There was a problem hiding this comment.
So... this is just passing a string directly through to fontnik.load, when the API requires passing a Buffer. Adding some argument validation (opts.font instanceof Buffer && opts.callback instanceof Function) at the beginning of makeGlyphs seems like a better approach than try/catch around fontnik.load or trying to change this upstream to pass an error through to the callback? If an attempt to read a font from disk fails in a downstream module, shouldn't that error be handled before trying to pass along something invalid to fontmachine?
throw errors and add tests for API misuse
|
Clarified the wording of the multi-face font error callback and added some checks that will throw if the API is misused, fix here and switch to |
@miccolis do you know how this branch is triggered so we could write test coverage for it?