parser-error vs user-error about primus

[3rd-Eden]

I didn't even realise that it would capture user thrown errors. Not sure if can be seen as a bug or as a feature, as I can that this can be useful but also really annoying.

I'm just going to label this as a bug for now. The quickest fix would be to force parsing to be async using a process.nextTick/setimmediate/setTimeout

Note: We could use https://github.com/NobleJS/setImmediate for this.

[gjohnson]

Yeah, emitting data on the next tick does the trick.

gjohnson@ace4052

[3rd-Eden]

Good to know that it's working, but not sure if that would be the optimal solution for this. I think it makes more sense to update the parsers https://github.com/3rd-Eden/primus/blob/master/parsers/json.js#L11 so they call the fn callback after the try / catch statement. This would also increase performance as V8 won't be de-optimising transformation handling.

[gjohnson]

Ohhhhh I see now... Good call, I'll go ahead and fix them that way. I don't really see the need to make the fn call async in this case now though. This what your thinking?

exports.decoder = function decoder(data, fn) {
  try { data = JSON.parse(data); }
  catch (e) { return fn(e); }
  fn(undefined, data);
}

Though, I guess you could also drop fn from the catch statement too... Need to test it, unless you know off the top of your head.

exports.decoder = function decoder(data, fn) {
  var err = undefined;
  try { data = JSON.parse(data); }
  catch (e) { err = e; }
  fn(err || undefined, data);
};

[3rd-Eden]

Yes, your first example was exactly what I was thinking. The second example would also work and can be written cleaner as well:

exports.decoder = function decoder(data, fn) {
  var err; // err is always undef when not nothing is assigned to it.

  try { data = JSON.parse(data); }
  catch (e) { err = e; }

  fn(err, data);
};

[gjohnson]

Ah with this fix, it shows an actual failed test when using binary-pack. When checking for \u2028 and \u2029 in https://github.com/3rd-Eden/primus/blob/master/spark.js#L206, currently it always assumes strings and uses packet.indexOf.

Want to just ignore anything that's not a string? Seems silly to cast the buffer right back to a string... Meh?

[3rd-Eden]

@gjohnson whoops, yes I think a simple typeof check would be sufficient to fix that.

[3rd-Eden]

Closing this. It was fixed in #43