802850 - Better debug output for context

Status: RESOLVED FIXED

(L20n :: JS Library, defect, P4)

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

can you provide some description for this bug Stas?

Comment 2

[Staś Małolepszy :stas]

Yeah, that description isn't the most helpful I've seen. :)

I meant revising how we emit events on errors, warnings and maybe (in some paranoiac-debug mode) on valid actions, like "got entity foo with value "Foo"."

Comment 3

[Staś Małolepszy :stas]

Attachment: Emit L20n warnings and errors, throw JavaScript errors

This was easier than I thought it would be once I discovered that since 2010 web consoles actually show errors thrown by asynchronous code.  D'oh.  (Bug 618078).

It turns out that the solution from bug 869372 was only half-good.  Providing an error callback in the promise chain was the right half, but instead of emitting the exception, we can simply throw it and the console will show it just fine.

The patch splits all errors into three buckets:

 - JavaScript errors - those are thrown in the errback in requestLocales,
 - serious context errors (unknown identifier, no resources) - those are thrown if the method is sync, or emitted as 'error' events otherwise,
 - recoverable context errors, all parser and compiler errors - those are emitted as 'warning' events

This allows the developer to do smart things like:

  var parserErrors = [];
  ctx.addEventListener('warning', function(e) {
    if (e instanceof L20n.Context.Error) {
      console.warn(e);
    }
    if (e instanceof L20n.Parser.Error) {
      parserErrors.push(e);
    }
    if (e instanceof L20n.Compiler.Error) {
      alert(e);
    }
  });

  ctx.addEventListener('error', console.error);

The patch removes all listeners from the bindings so the default console output is as clean as it could be.

I'll add tests tomorrow, but I think this is good to review now.

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Staś Małolepszy :stas from comment #3)


>  - serious context errors (unknown identifier, no resources) - those are
> thrown if the method is sync, or emitted as 'error' events otherwise,

Why do we want to have different behavior depending on sync/async here?

Comment 5

[Staś Małolepszy :stas]

It seems like a good practice to give feedback as soon as possible.  Throwing in sync methods is the most natural thing a developer can expect.   OTOH, the develoer is not able to catch errors thrown in async methods, so again, the most natural things is to listen to error events.  Another common practice is to pass the error object as the first argument to callbacks, but our async methods don't take callbacks usually.

One possible improvement in order to make this more consistent would be to both throw and emit in sync methods, although I'm not convinced that's needed.  I mean, if you don't catch it, your code will stop anyways, so emitting doesn't help much at this point.

Comment 6

[Zibi Braniecki [:zbraniecki][:gandalf]]

the problem with this distinction is that the cost of switching sync/async is much greater as it requires rewriting the code logic.

(and again, as a side note - why do we keep sync? :))

Comment 7

[Staś Małolepszy :stas]

Which cost?  For us?  For developers?

The methods that throw are for instance:

 - get[Entity] when the context isn't ready, 
 - updateData when the argument isn't an object,
 - localize without a callback,
 - [add|link]Resource when the context is frozen,
 - registerLocales and registerLocaleNegotiator when the context is frozen,
 - registerLocales when you pass a non-string,
 - requestLocales when the context doesn't have any resources yet.

Surely, these justify synchronicity?  Would you rather emit?

Comment 8

[Zibi Braniecki [:zbraniecki][:gandalf]]

I mean the cost for the developer.

Scenario I have on mind is when the dev writes code that reacts to those errors (say, editor, or some less-dumb reaction to missing entities/locales) and then he wants to experiment with sync/async impact on perf.

In this model he has to rewrite the code that reacts and bind it differently depending on if we throw or emit.

My *gut feeling* is that we should emit in both cases. But I'm ready to be proven wrong.

Comment 9

[Staś Małolepszy :stas]

These methods are not sometimes sync and sometimes async.  There is no choosing between sync/async modes.  Hence, in my mind, there is not cost of switching.  You never switch.

get and getEntity are always sync, and so they throw.  updateData, addResource and linkResource are sync.  registerLocales and registerLocaleNegotiator are sync.  For these methods, I believe we should throw.

The two methods that are more interesting are localize and requestLocales.  Here, my reasoning is as follows:  if an error occurs in the async "routine" that one of these methods executes, then we emit.  But there are some pre-conditions that we can check before we even start executing that async routine.  Like, if you didn't pass a callback to localize(), or you call requestLocales(3) (instead of passing a string).  They are akin to syntax error.  I think we should throw in these cases; otherwise debugging is harder.

Comment 10

[Staś Małolepszy :stas]

I think I found a good analogy.  Node's Redis client is asynchronous, but some methods do some first-pass check of the syntax (e.g. arguments passed) and throw synchronously.

See the errors in send_command:

https://github.com/mranney/node_redis/blob/v0.8.4/index.js#L691-L693

  throw new Error("First argument to send_command must be the command name string, not " + typeof command);
  throw new Error("send_command: last argument must be a callback or undefined");
  throw new Error("send_command: second argument must be an array");

On the other hand, if the error comes from Redis itself (like, you try  authenticate to the server with a wrong password), the client will call the callback with the error object as the first argument.

I like this approach and I'd like to do something similar for our async methods.

Comment 11

[Staś Małolepszy :stas]

Attachment: Patch with tests

The code changes are relatively few, most of the patch are tests for locale fallback and accompanying events.

Here is the breakdown of the errors and warnings that we emit and log in the console by default.  This is in line with Gandalf's idea to log broken translations as errors and missing translations as warnings.  The reason is that a developer or a localizer will likely want to see when they've broken an entity (syntax error, reference error) as their intent was to have that entity show up.  If an entity is missing from the resource completely, then we only emit a warning which can be filtered nicely in the console.  This way, you can start localizing with a small number of entities in your locale and we will only show you errors if what you typed in the resource file is wrong.

 - `error` - fired when an error occurs which prevents the `Context` instance 
   from working correctly or when the existing translations are buggy.  The 
   error object is passed as the first argument to `callback`.  These errors 
   include:

   - native JavaScript errors (`TypeError`, `ReferenceError` etc.), (this might get changed by bug 869016)

   - `Context.RuntimeError`, when an entity is missing or broken in all 
     supported locales;  in this case, L20n will show the the best available 
     fallback of the requested entity in the UI:  the source string as found in 
     the resource, or the identifier.

   - `Context.TranslationError`, when the translation is present but broken 
     in one of supported locales;  the `Context` instance will try to retrieve 
     a fallback translation from the next locale in the fallback chain,

   - `Parser.Error`, when L20n is unable to parse the syntax of a resource; 
     this might result in entities being broken which in turn can lead to above 
     error being emitted.

 - `warning` - fired when a less serious error occurs from which the `Context` 
   instance can recover gracefully and try to fetch a translations from 
   a fallback locale.  The error object is passed as the first argument to 
   `callback` and can be one of the following:

   - `Context.Error`, when there are problems with setting up resources (e.g. 
     a 404 error when fetching a resource file, or recursive `import` 
     statements in resource files),

   - `Context.TranslationError`, when there is a missing translation 
     in one of supported locales;  the `Context` instance will try to retrieve 
     a fallback translation from the next locale in the fallback chain,

   - `Compiler.Error`, when L20n is unable to evaluate an entity to a string; 
     there are two types of errors in this category: `Compiler.ValueError`, 
     when L20n can still try to use the literal source string of the entity as 
     fallback, and `Compiler.IndexError` otherwise.

Comment 12

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 793990 [details] [diff] [review]
Patch with tests

Review of attachment 793990 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/l20n/context.js
@@ +412,5 @@
> +            // evaluate completely;  this is still better than returning the 
> +            // identifer;  prefer a source string from locales earlier in the 
> +            // fallback chain, if available
> +            var source = prevSource || { source: e.source, loc: locale.id };
> +            return getFromLocale.call(this, cur + 1, id, data, source);

you're creating a variable to use it only once. Maybe just pass it as an argument?

Comment 13

[Staś Małolepszy :stas]

Thanks, Gandalf! I'm really happy that we fixed this!  Landed in https://github.com/l20n/l20n.js/commit/f1727f8b58114c5b3ec9f00bc276832b5080e5d5

(In reply to Zbigniew Braniecki [:gandalf] from comment #12)

> you're creating a variable to use it only once. Maybe just pass it as an
> argument?

It's for readability and because the comment ("prefer…") relates to the "var source…" line.  I kept it in, there's no performance impact.