chore: clean up a few things around the script element check

Rename the flag to `allow_self_closing_script`, which may be verbose,
but it's also not recommended to use anyway. And it feels a bit more
descriptive, we don't ignore script errors, but allow the error of a
self-closing script tag.

Ensure that the configuration from the config file as well as from the
CLI gets evaluated.

Introduce an "options" struct, making future enhancements a bit simpler.

Use plain strings for errors, vs concatenating and formatting.

Author: ctron

src/common/html_rewrite.rs

@@ -1,6 +1,12 @@
-use anyhow::{Error, Result};
+use anyhow::{bail, Result};
 use lol_html::{element, html_content::Element, HtmlRewriter, Settings};
 
+#[derive(Clone, Debug, Default)]
+pub struct DocumentOptions {
+    /// Ignore self-closing script warnings
+    pub allow_self_closing_script: bool,
+}
+
 /// A wrapper for Html modifications, and rewrites.
 #[derive(Debug)]
 pub struct Document(Vec<u8>);
@@ -14,33 +20,26 @@ impl AsRef<[u8]> for Document {
 impl Document {
     /// Create a new document
     ///
-    /// Note: if this is not a valid HTML document, it will fail later on.
-    // In case we want to add more things to check in the future, we can take in the config as an
-    // argument, and `anyhow::Result<()>` could be
-    // replaced with an `std::result::Result<(), MyErrorEnum>` in case we want to handle the error
-    // case differently depending on the error variant.
-    pub fn new(data: impl Into<Vec<u8>>, ignore_script_error: bool) -> Result<Self> {
+    /// This will fail if a non-valid HTML is provided or a self-closing script element is found
+    /// and the self-closing script element is not allowed in the options.
+    pub fn new(data: impl Into<Vec<u8>>, options: DocumentOptions) -> Result<Self> {
         let doc = Self(data.into());
 
-        // Check for self closed script tags such as "<script.../>"
-        doc.select("script[data-trunk]", |el| {
+        // Check for self-closed script tags such as "<script.../>"
+        doc.select("script", |el| {
             if el.is_self_closing() {
-                const SELF_CLOSED_SCRIPT: &str = concat!(
-                    "Self closing script tag found. ",
-                    r#"Replace the self closing script tag ("<script .../>") with a normally closed one such as "<script ...></script>"."#,
-                    "\nFor more information please take a look at https://github.com/trunk-rs/trunk/discussions/771."
-                );
-
-                if ignore_script_error {
-                    tracing::warn!("{}", SELF_CLOSED_SCRIPT);
+                if options.allow_self_closing_script {
+                    tracing::warn!("Self-closing script tag found (allowed by configuration)");
                 }
                 else {
-                    return Err(Error::msg(
-                        format!("{}\n{}", 
-                            SELF_CLOSED_SCRIPT,
-                            r#"In case this is a false positive the "--ignore-script-error" flag can be used to issue a warning instead."#
-                        )
-                    ))
+                     bail!(
+                        r#"Self-closing script tag found.
+
+Replace the self-closing script tag ("<script .../>") with a normally closed one such as "<script ...></script>".
+For more information, please take a look at https://github.com/trunk-rs/trunk/discussions/771."
+
+In case this is a false positive, the "--allow-self-closing-script" flag can be used to issue a warning instead."#
+                    )
                 }
             }
             Ok(())
@@ -147,19 +146,25 @@ impl Document {
 mod test {
     use super::*;
 
+    /// Run some basic tests with a spec-compliant HTML file.
+    ///
+    /// The focus is on the `<script>` element, and around other self-closing elements. If a
+    /// self-closing script tag is being used which, according to the spec should not be used, bad
+    /// things may happen. This test is there to test our expectation towards a spec-compliant file.
     #[test]
     fn test_script_spec() {
         let mut doc = Document::new(
             r#"
 <html>
     <head>
+        <link/>
         <script href="test"></script>
         <link>
     </head>
     <body></body>
 </html>
 "#,
-            false,
+            Default::default(),
         )
         .expect("this is valid HTML");
 
@@ -173,6 +178,7 @@ mod test {
             r#"
 <html>
     <head>
+        <link/>
         <script href="test"><span>here</span></script>
         <link>
     </head>
@@ -182,9 +188,17 @@ mod test {
         );
     }
 
+    /// Ensure we get an error for any self-closing script tag
     #[test]
     fn test_self_closing_script_tag() {
-        let doc = Document::new("<script data-trunk/>", false);
+        let doc = Document::new("<script/>", Default::default());
+        assert!(doc.is_err());
+    }
+
+    /// Ensure we get an error for a self-closing trunk script tag.
+    #[test]
+    fn test_self_closing_trunk_script_tag() {
+        let doc = Document::new("<script data-trunk/>", Default::default());
         assert!(doc.is_err());
     }
 }

src/config/models/build.rs

@@ -143,11 +143,11 @@ pub struct ConfigOptsBuild {
     #[arg(long)]
     pub no_sri: bool,
 
-    /// Ignore error's related to self closing script elements, and instead issue a warning.
+    /// Ignore error's related to self-closing script elements, and instead issue a warning.
     ///
-    /// Since this error can cause the HTML output to be truncated, only enable this in case you
+    /// Since this issue can cause the HTML output to be truncated, only enable this in case you
     /// are sure it is caused due to a false positive.
     #[serde(default)]
     #[arg(long)]
-    pub ignore_script_error: bool,
+    pub allow_self_closing_script: bool,
 }

src/config/models/mod.rs

@@ -362,6 +362,10 @@ impl ConfigOpts {
                 if l.no_sri {
                     g.no_sri = true;
                 }
+                // NOTE: this can not be disabled in the cascade.
+                if l.allow_self_closing_script {
+                    g.allow_self_closing_script = true;
+                }
 
                 Some(g)
             }

src/config/rt/build.rs

@@ -78,8 +78,8 @@ pub struct RtcBuild {
     pub minify: Minify,
     /// Allow disabling SRI
     pub no_sri: bool,
-    /// Ignore error's due to self closed script tags, instead will issue a warning.
-    pub ignore_script_error: bool,
+    /// Ignore error's due to self-closed script tags, instead will issue a warning.
+    pub allow_self_closing_script: bool,
 }
 
 impl RtcBuild {
@@ -185,7 +185,7 @@ impl RtcBuild {
             accept_invalid_certs: opts.accept_invalid_certs,
             minify,
             no_sri: opts.no_sri,
-            ignore_script_error: opts.ignore_script_error,
+            allow_self_closing_script: opts.allow_self_closing_script,
         })
     }
 
@@ -228,7 +228,7 @@ impl RtcBuild {
             accept_invalid_certs: None,
             minify: Minify::Never,
             no_sri: false,
-            ignore_script_error: false,
+            allow_self_closing_script: false,
         })
     }
 

src/pipelines/html.rs

@@ -1,7 +1,7 @@
 //! Source HTML pipelines.
 
 use crate::{
-    common::html_rewrite::Document,
+    common::html_rewrite::{Document, DocumentOptions},
     config::{RtcBuild, WsProtocol},
     hooks::{spawn_hooks, wait_hooks},
     pipelines::{
@@ -83,7 +83,12 @@ impl HtmlPipeline {
 
         // Open the source HTML file for processing.
         let raw_html = fs::read(&self.target_html_path).await?;
-        let mut target_html = Document::new(raw_html, self.cfg.ignore_script_error)?;
+        let mut target_html = Document::new(
+            raw_html,
+            DocumentOptions {
+                allow_self_closing_script: self.cfg.allow_self_closing_script,
+            },
+        )?;
         let mut partial_assets = vec![];
 
         // Since the `lol_html` doesn't provide an iterator for elements, we must use our own id.