Feature request: Don't serve or export page when the page value is `nil`

[stelcodes]

Proposal

In the static site generator that I'm building which uses Stasis, I want to be able to avoid exporting and serving a page by having a nil page value or if the page function returns nil. I only know if a respective page should be rendered after I do some expensive Markdown processing work. I would like to defer this work until each page needs to be rendered.

This would benefit users by allowing the page's function to not only dynamically load the content but also dynamically decide if the page should render at all.

No worries if you aren't feeling this, I can easily grab the source and change it for my needs. I just want to see if you like the idea. It does technically cause a breaking change in behavior, although small.

(use 'stasis.core)

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;; CURENT BEHAVIOR ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

(export-pages {"/" nil} "/tmp/stasis")
; (out) #error {
; (out)  :cause nil
; (out)  :via
; (out)  [{:type java.lang.NullPointerException
; (out)    :message nil
; (out)    :at [stasis.core$realize_page invokeStatic core.clj 44]}]
; (out)  :trace
; (out)  [[stasis.core$realize_page invokeStatic core.clj 44]
; (out)   [stasis.core$realize_page invoke core.clj 40]
; (out)   [stasis.core$export_page invokeStatic core.clj 127]
; (out)   [stasis.core$export_page invoke core.clj 124]
; (out)   [stasis.core$export_pages invokeStatic core.clj 146]
; (out)   [stasis.core$export_pages doInvoke core.clj 142]

(export-pages {"/" (constantly nil)} "/tmp/stasis")
; (out) #error {
; (out)  :cause No method in multimethod 'do-copy' for dispatch value: [nil java.io.FileOutputStream]
; (out)  :via
; (out)  [{:type java.lang.IllegalArgumentException
; (out)    :message No method in multimethod 'do-copy' for dispatch value: [nil java.io.FileOutputStream]
; (out)    :at [clojure.lang.MultiFn getFn MultiFn.java 156]}]
; (out)  :trace
; (out)  [[clojure.lang.MultiFn getFn MultiFn.java 156]
; (out)   [clojure.lang.MultiFn invoke MultiFn.java 238]
; (out)   [clojure.java.io$copy invokeStatic io.clj 406]
; (out)   [clojure.java.io$copy doInvoke io.clj 391]
; (out)   [clojure.lang.RestFn invoke RestFn.java 425]
; (out)   [stasis.core$export_page invokeStatic core.clj 140]
; (out)   [stasis.core$export_page invoke core.clj 124]
; (out)   [stasis.core$export_pages invokeStatic core.clj 146]
; (out)   [stasis.core$export_pages doInvoke core.clj 142]

(-> (serve-pages {"/" nil}) (apply [{:uri "/"}]))
; (out) #error {
; (out)  :cause nil
; (out)  :via
; (out)  [{:type java.lang.NullPointerException
; (out)    :message nil
; (out)    :at [stasis.core$realize_page invokeStatic core.clj 44]}]
; (out)  :trace
; (out)  [[stasis.core$realize_page invokeStatic core.clj 44]
; (out)   [stasis.core$realize_page invoke core.clj 40]
; (out)   [stasis.core$serve_after_finding_all_dependent_pages invokeStatic core.clj 98]
; (out)   [stasis.core$serve_after_finding_all_dependent_pages invoke core.clj 96]
; (out)   [clojure.core$partial$fn__5912 invoke core.clj 2654]
; (out)   [stasis.core$try_serving_dependent_page invokeStatic core.clj 89]
; (out)   [stasis.core$try_serving_dependent_page invoke core.clj 80]
; (out)   [stasis.core$serve_pages$fn__5375 invoke core.clj 118]

(-> (serve-pages {"/" (constantly nil)}) (apply [{:uri "/"}]))
;; {:status 200, :body nil, :headers {"Content-Type" "text/html"}}

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;; PROPOSED BEHAVIOR ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

(export-pages {"/" nil} "/tmp/stasis")
;; and
(export-pages {"/" (constantly nil)} "/tmp/stasis")
;; act like
(export-pages {} "/tmp/stasis") ; nil

(-> (serve-pages {"/" nil}) (apply [{:uri "/"}]))
;; and
(-> (serve-pages {"/" (constantly nil)}) (apply [{:uri "/"}]))
;; act like
(-> (serve-pages {}) (apply [{:uri "/"}]))
; {:status 404,
;  :body "<h1>Page not found</h1>",
;  :headers {"Content-Type" "text/html"}}

[magnars]

This is an interesting philosophical question. There are two reasons why I might be getting nil:

1. Intentionally, the page shouldn't exist
2. Unintentionally, I've made a mistake

The way Stasis works right now is not good in either case. For case 1) it breaks. For case 2) it gives bad error messages.

The backwards compatible fix would be creating good error messages. The breaking change would give us a more lenient API that's nice to use for case 1), but could result in some serious outage issues in case 2). Consider a rogue (some-> taking down the entire site.

Is there some way we could handle both cases without breaking the API? Maybe add an optional options parameter to these functions, making "nil punning" an opt-in feature?

[cjohansen]

My first thought when reading this was your last suggestion @magnars: Add an optional map with a parameter that specifies whether nils are expected or not when exporting. So 👍 from me!

[stelcodes]

@magnars @cjohansen Thanks for considering this! An optional options map would be great. I'll try adding that to the codebase. I should probably have it done in the next few weeks :)

[stelcodes]

@magnars @cjohansen

I started working on this and I need some feedback. I'm using :ignore-nil-pages? as an option name. I feel like there are two ways to go about this:

1. Include :ignore-nil-pages? in the current optional map that is used as the context. Pros: No added arguments to export-pages and serve-pages Cons: Possibly breaking change to someone already using that keyword, option is passed into the page functions and ring request maps with no clear benefit, just clutter.
2. Add another optional map just for Stasis options so the new functions will be like this: serve-pages [get-pages & [options new-options]] Pros: Surely won't break anyone, doesn't clutter context map and ring request maps Cons: Now there are two maps called options in the code.

I feel like option 2 is better because it 100% won't break any existing code, but there is a naming problem. Currently the first optional map is called options everywhere in the code, and I think if we're going to add another options map we should rename the former to context or config. context fits great with the code examples where the page functions accept an argument called cxt. Any thoughts?

[magnars]

What do you think of 1) with a namespaced key? :stasis/ignore-nil-pages?

[stelcodes]

@magnars I like that!

[stelcodes]

Ok the PR is ready for review :cowboy_hat_face: