pass queue errors to next middleware from view

[qwales1]

This is in reference to issue #1249. It passes out errors from initQueue to the next middleware. I am not sure if this is something you want to implement as it changes the View function signature to include next as a third parameter and stores a reference to next within the view, but wanted to take a stab and it and see how it could be done.

[creynders]

Some remarks:

• I'd drop the default value for this.next and add a check to the conditional:

if(err && this.next){
  this.next(err);
}else{
//...

• It has a possible side-effect we need to think through: if any of the pre("render") middleware throws an error, it will be passed along too. I'm not entirely sure this is always wanted.
• In general: it would be cool if you could try and clean up all the small code formatting changes (tabs swapped to space, semi-colon dropped, ...) Obviously for code hygiene, but also since it makes the PR easier to read, i.e. see what exactly has changed.
• I don't think the signature change is a big problem, since it's backwards compatible
• if it has no side-effects I'm 👍 with the PR, thanks!

[qwales1]

Awesome thanks for looking at that. In response to your remarks:

1. Agree, much nicer with the conditional
2. I didn't think about this, but I will definitely take a closer look and see if there is a way to account for both situations.
3. Ooops sorry about that. I thought I configured the editor correctly. I will clean that up.

I'll try and get those changes done soon. Thanks!

On Mar 26, 2015, at 1:00 AM, creynders [email protected] wrote:

Some remarks:

I'd drop the default value for this.next and add a check to the conditional:
if(err && this.next){
this.next(err);
}else{
//...
It has a possible side-effect we need to think through: if any of the pre("render") middleware throws an error, it will be passed along too. I'm not entirely sure this is always wanted.
In general: it would be cool if you could try and clean up all the small code formatting changes (tabs swapped to space, semi-colon dropped, ...) Obviously for code hygiene, but also since it makes the PR easier to read, i.e. see what exactly has changed.
I don't think the signature change is a big problem, since it's backwards compatible
if it has no side-effects I'm with the PR, thanks!
—
Reply to this email directly or view it on GitHub.

[qwales1]

@creynders I looked at the effect on the pre('render') queue and I couldn't think of a way to distinguish the errors between those and the initQueue . Some possible solutions I came up with:

1. Add to the documentation that if you want errors that occur when rendering this view to be handled by middleware then pass next as the third parameter when initializing the view.
2. Change this PR to still include the change to the function signature and store a reference to next in the view, but not call this.next unless an option is also passed to the view. Something like var view = new keystone.View(req,res,next, {renderOnError: false}) so you can explicitly buy in to this behavior.
3. Scrap this PR and maybe make a module that extends view to have this behavior. I tried briefly, but haven't gotten it to work yet.

What do you think?

[creynders]

I think 1. should be good enough. However, it does change functionality implicitly. Need to consult @JedWatson on this one. Thanks for the follow-up!