issue144: using winston to keep the code impact more local.

[monkeyiq]

The ufoJS code now uses a misc.logger to tell when bad things happen during the load. This removes the passing of callbacks down the function chain.

The default ufoJS is to rethrow the exception case that was passed to logger. This should give the same behaviour as before the PR. Notice that in makeRethrowLogger() he original meta.err is thrown, so the stack trace should look as one would expect.

Code like MP can patch the misc.logger to their own logger (which I have a separate PR todo) and thus record failures and allow the program to continue loading.

If this looks like a solution that might be merged then I'll do more testing on it than the few runs so far. This replaces https://github.com/graphicore/ufoJS/pull/12.

One downside to the current code is that the logger is ufoJS wide. It would be nice to be able to patch the logger on a GlyphSet basis and leave other GlyphSets using the unpatched logger instance.

[graphicore]

Did you test winston in the browser? use the serve.sh script to access the the testsuite in the browser, all of ufoJS must also run in the browser (after all, that's the whole point of porting robofab to javascript) .

I'm asking because https://github.com/flatiron/winston doesn't mention browsers nor require.js

[graphicore]

Maybe, we can use the old pull request, but set up a document which discusses our requirements for a good logger. A logging mechanism that could be used in metapolator and ufoJS, browser and nodeJS, (and synchronously and asynchronously) I don't want to block your task for too long, but in exchange for a sub-optimal solution, we should start thinking about a robust solution, that will solve all our needs in this respect. @rrthomas thoughts?

[monkeyiq]

we could also use the style in this PR but replace the use of winston with just a callback function. The default cb would be to throw the error, if you override it then you could set the error handler. This is closer to the first PR but avoids passing the cb down the function stack.

ie, misc/logger: _makeRethrowLogger() would become just a cb which does the throw. no winston needed.

[rrthomas]

As I wrote elsewhere, it seems we currently have three different uses for logging:

1. Debugging. Output via console.log is fine for this, either on the command-line or in the browser.
2. Immediate output in the user interface. The same style as console.log is fine, it just needs a different call.
3. Collection for later inspection in the user interface. It's good if this can be more structured; in the current example, it's good if you can look at a particular glyph and see why it didn't import, rather than having to scan down a log of messages, or save a font project with embedded error information. However, this is only "nice to have": looking down a list is still fine, and in some ways is better, if e.g. you consider the list to be a list of "todo" items.

Maybe we should stick to 1 & 2 for the moment? Then, all we need is a replacement for console.log. I suggest that it should a) call console.log and b) append a message to an in-memory log.

I would suggest actually hijacking console.log, as this avoids having to document a new API, and means that finger habits aren't upset; and existing code would automatically use it.

It seems to me we don't need Winston for this: the whole thing should only be a few lines of code, to a) replace the console.log method; b) write out a log as YAML and c) read it in. If one wanted to adopt more complex logging in future, it would be no harder to change to that than from the existing code.

[monkeyiq]

So maybe using callbacks (either in a globalish scope like misc as in this PR) or directly passing them down to validateAndMassagePointStructures() through the intermediate functions as in the previous PR might be the way to solve the current issue.

Basically we want validateAndMassagePointStructures() to be able to call a reporting method somehow and then have code to try to recover from errors. The default "reporting method" is just to throw an exception, this will preserve the functionality that is currently in place in ufojs. When ufojs sees something it doesn't like it throws an exception. If the reporting method is replaced, as the metapolator code would do using either of the 2 styles above, or just winston, or some other way, then the new reporting method can just log the error that happened and allow the ufoJS code to recover. Where 'recover' might involve dropping the outlines for a glyph which ufojs doesn't understand or like.

I'm happy to keep using console.log for logging.

Though we can't really hijack console.log to implement the throw exceptions for specific log messages design that winston could handle. With winston it was ok because there could be a winston object for ufojs itself. And ufojs probably doesn't want to hijack console.log itself in order to throw exceptions for load errors.

A simpler design which is what I had at the very first is to just avoid loading anything for the glyph when there is any error. Record that in the YAML and move on. This can be done all within just ImportController.import() and avoids any change to ufojs. Doing this we can only know the glyphName that failed, no other metadata from the glif file will be known (for example the unicode hex).

[monkeyiq]

Maybe just doing the method from the last paragraph in the above message is the way? This gets the task complete.