8000 Missing beforeResponse hook on some error responses · Issue #3254 · nitrojs/nitro · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Missing beforeResponse hook on some error responses #3254

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
cjpearson opened this issue Mar 25, 2025 · 2 comments
Open

Missing beforeResponse hook on some error responses #3254

cjpearson opened this issue Mar 25, 2025 · 2 comments
Labels
pending triage

Comments

@cjpearson
Copy link
Contributor

Environment

Nitropack@2.11.7 h3@1.15.1

Reproduction

https://stackblitz.com/edit/unjs-nitro-starter-ppkblahg?file=server%2Froutes%2Fthrow.ts

Describe the bug

Depending on how an error is caused, the beforeResponse hook may not be called.

Here are a few examples from the reproduction

Error creation onError onBeforeResponse onAfterResponse
return new Error() yes yes no
throw new Error() yes no no
missing route yes no no
new Response(null, { status: 503 }) no yes yes

I would expect the beforeResponse hook to be called even for missing routes or thrown errors to allow intercepting those responses.

Digging through the code, it looks like the root cause may be that nitro's onError handler actually sends the response https://github.com/nitrojs/nitro/blob/v2/src/runtime/error.ts#L82

This marks the event as handled, so h3 will not call the beforeResponse/afterResponse hooks https://github.com/unjs/h3/blob/v1/src/adapters/node.ts#L73

Maybe a solution could be to also trigger those hooks within the error handler, before and after the send calls.

Additional context

Follow up from #2560

Logs


      		
    

      
  





        



            
            

              
            

        

      


      
    








      

    



      

  
            


  

  

    
  


  

      

  This was referenced Mar 25, 2025





  


  

      
  
  Closed

  






      






  


  

      
  
  Open

  






  









      

  
      



  

  @pi0





  


    

      

  

    

        
    
    

  


      
          
  
      
            Copy link

  

      
    

  


  

      

  
  Member


      

  


  
    

      

      
            pi0
  

      

      

      commented


        Mar 25, 2025


      
    


  




      


        


  
    

      
          
Thank you so much for spending time to make this info.


In h3 v1 / nitro v2


There is a PR to change the order of calling hooks onBeforeResponse > onError (#793) , which, while it is a completely valid fix, also changes behavior, and at this point of v1 adoption, it is way risky to change it at least without extensive ecosystem tests.


For Nitro v2, (with h3 v1), I like your idea could instead trigger the necessary hook from within the error handler. Since Nitro 2.11 it is possible to stack multiple error handlers for this purpose.


In h3 v2 / nitro v3


onAfterResponse is not supported in h3 v2 since send() does not exist outside of Node.js runtime (and for Node.js runtime, we can use event.node.res.on("..", () => {}) as another workaround)


I think what could be best to investigate and possibly fix any issues before h3 v2 release is that we make sure onBeforeResponse is called all the times (for normal, empty, error or 404 responses).

      		
    

  





        


            

            
            

              
            

        

      


      

            
  
  

    
  
    
    

  

  

  


    












      

  
      



  

  @cjpearson





  


    

      

  

    

        
    
    

  


      
          
  
      
            Copy link

  

      
    

  


  

      

  
    Contributor


      

  Author


  


  
    

  




      


        


  
    

      
          


There is a PR to change the order of calling hooks onBeforeResponse > onError (#793) , which, while it is a completely valid fix, also changes behavior, and at this point of v1 adoption, it is way risky to change it at least without extensive ecosystem tests.




Yeah I saw this PR, and understand that it may be risky at this point. Since this issue a result of the combination of this behavior plus nitro's error handler I thought it made sense to report here first.




For Nitro v2, (with h3 v1), I like your idea could instead trigger the necessary hook from within the error handler. Since Nitro 2.11 it is possible to stack multiple error handlers for this purpose.




Are you open to a PR for this change or do you mean this should be done within applications?




I think what could be best to investigate and possibly fix any issues before h3 v2 release is that we make sure onBeforeResponse is called all the times (for normal, empty, error or 404 responses).




Yeah that makes sense.


Maybe we could also clarify the expected behavior of the error hook? My original assumption was that it was the equivalent to beforeResponse for error responses, but then I looked through the code and saw it was not the case. Perhaps it could be documented that this hook is best for logging errors, but if you need to actually make changes to the response use beforeResponse and check the status. Otherwise you run into issues like #2771


The other question I had is should the error hook also be called for something like new Response(null, { status: 503 })? Or is it just for JS errors, either thrown or returned.

      		
    

  





        


            

            
            

              
            

        

      


      

            
  
  

    
  
    
    

  

  

  


    
















  
  

    

      

  





  




  
  

              

  
    Sign up for free
    to join this conversation on GitHub.
    Already have an account?
    Sign in to comment


  



  






  
                  




    

  


      
  

    Assignees
  



        

    No one assigned







      


  


  

    Labels
  



    

      

    pending triage









      

  

    

        

    Projects
  


        




    None yet




  



      


  

    
  

    Milestone
  


      No milestone





    
      
          


  

    

      
        

          
    

    Development
  




            
    
No branches or pull requests







      
    

  




      

    

  
      

  

    

      2 participants
    

    

        
          @cjpearson 
        
          @pi0 
    

  






    

    




    






  

          

        



    



  





    

  


  


          



    



  

    

    

    





    


    

    

  



0