Consider spliting validate in two methods

[neumino]

I thought it was insane to do it, and now I kind of regret it.
We should split validate in two methods, one synchronous and one asynchronous

See #113 (comment)

[ThisIsMissEm]

Why not just make the synchronous one look or act async?

i.e.,

exports.operation = function(args…, callback){
  if(syncOp){
     // do sync work, return [err, result]
     process.next(function(){ callback(err, result); })
  } else {
     asyncOp(args…, callback)
  }
}

See this from the Node.js manual/docs: http://nodejs.org/docs/latest/api/process.html#process_process_nexttick_callback

[ThisIsMissEm]

You could also push the entire // do sync work into the process.nextTick which would be most advisable.

[chrisfosterelli]

Perhaps it's also a good time to point out that the behaviour of validate seems a little opinionated. Throwing an exception is a huge pain to work with, and failing validation is not really an "exceptional" circumstance. I'd argue a simple return value is better behaviour. What do you think?

[neumino]

@chrisfosterelli -- why is it a huge pain? You just have to wrap it in a try/catch. It's the same as a if/else.
Also, in the most common case, you don't need to call validate yourself, it is automatically called when you execute save for example.

[chrisfosterelli]

@neumino throwing errors in Javascript is kind of considered a bad pattern unless it's something that's actually unexpected. Invalid data isn't an exceptional case.

Not calling validate is what caused my problem in the first place. When I called save, I was imagining that the validation failure would simply reject the promise (as would be expected behaviour for promises), not crash the app with an exception...

I've been playing around with the validation logic now, and it seems that the promise is rejected in some cases but an exception is thrown in others. If I pass in an invalid date, an exception is thrown. The other cases seem to reject the promise. Is this expected behaviour?

[neumino]

@chrisfosterelli -- There is a try/catch wrapping the validate in save, so it shouldn't throw
https://github.com/neumino/thinky/blob/master/lib/document.js#L613

If it does, could you provide a script to reproduce that? save should not throw.

Also as far as I know, it's not a bad pattern to throw. It's bad to throw when you don't catch it, and/or don't set up a domain. From https://www.joyent.com/developers/node/design/errors

The next most common case is an operational error in a synchronous function like JSON.parse. For these functions, if you encounter an operational error (like invalid user input), you have to deliver the error synchronously. You can either throw it (much more common) or return it.

Also in the case of validate, the reason throwing make more sense is because the function is recursive, and it's a pain to return the error. You basically have to add if (error) { return error } everywhere.

[chrisfosterelli]

@neumino The difference for me would be that the responsibility of JSON.parse is to "parse this string to JSON and return the result". The function cannot do it's job when it's invalid input, so it throws. It also can't use the return value to pass the error because it would be impossible to differentiate between valid JSON values (like null or -1) and an actual error condition. The job of the validate function is to "return if this model is valid or not", at least to me. My reasoning for that is mostly based on sources like this and this one.

However, you're definitely right that the nodejs docs appear to explicitly outline user input validation as one of the only use cases for throwing, so that seems like fair reasoning to me then!

I won't distract this thread anymore from considering whether or not to split validate into two methods, I've created issue #163 for the problem I appear to be having with the promise. Thanks!

[neumino]

Thanks @chrisfosterelli for chiming in. It makes sense, I can see why returning a value can make more sense.

Currently thinky returns the first error, and some people would prefer to return all of them. Maybe that would be a good time to revisit whether validate should throw or return a value since return multiple errors is kind of cumbersome.

[neumino]

That was kind of done some time ago.