Do not consider http 304 a request failure (#126)

* Do not consider http 304 a request failure. This status indicates the resource was not modified, rather than being moved. It's used for caching, and considering it a request failure means the server must disable all cache-related headers when rendering for grover.
Studiosity · Aug 27, 2021 · f0f6c1c · f0f6c1c
1 parent e4296b5
commit f0f6c1c
Show file tree

Hide file tree

Showing 2 changed files with 16 additions and 1 deletion.
diff --git a/lib/grover/js/processor.js b/lib/grover/js/processor.js
@@ -127,7 +127,7 @@ const _processPage = (async (convertAction, urlOrHtml, options) => {
     const raiseOnRequestFailure = options.raiseOnRequestFailure; delete options.raiseOnRequestFailure;
     if (raiseOnRequestFailure) {
       page.on('requestfinished', (request) => {
-        if (request.response() && !request.response().ok() && !request.redirectChain().length > 0) {
+        if (request.response() && !(request.response().ok() || request.response().status() == 304) && !request.redirectChain().length > 0) {
           errors.push(request);
         }
       });

diff --git a/spec/grover/processor_spec.rb b/spec/grover/processor_spec.rb
@@ -592,6 +592,21 @@
           end
         end
 
+        context 'when a 304 occurs it does not raise an error' do
+          let(:url_or_html) do
+            <<-HTML
+              <html>
+                <body>
+                  Hey there
+                  <img src="https://httpstat.us/304" />
+                </body>
+              </html>
+            HTML
+          end
+
+          it { expect(pdf_text_content).to include 'Hey there' }
+        end
+
         context 'when assets have redirects PDFs are generated successfully' do
           it { expect(pdf_text_content).to match "#{date} Google" }
         end