Carry a script's top-level declarations across the wombat block

CHANGELOG.md

@@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Fixed
+
+- Carry a script's top-level `const`, `let` and `class` declarations across the wombat block, so other scripts on the page can still see them (#329)
+
 ## [5.4.1] - 2026-07-31
 
 ### Fixed

pyproject.toml

@@ -34,6 +34,13 @@ dependencies = [
   "piexif==1.1.3", # this dep is a nightmare in terms of release management, better pinned just like in optimize-images anyway
   "idna>=2.5,<4.0",
   "xxhash>=2.0,<4.0",
+  # Parsing JavaScript well enough to know which names a script declares at its
+  # top level (see rewriting/js_ast.py). tree-sitter rather than a pure-Python
+  # parser because the input is whatever the live web served: esprima is ES2017
+  # and refuses optional chaining, class fields and `for await`, all ordinary in
+  # shipped code, and a parse failure here silently restores the bug this fixes.
+  "tree-sitter>=0.23,<1.0",
+  "tree-sitter-javascript>=0.23,<1.0",
   "types-xxhash>=2.0,<4.0",
 ]
 dynamic = ["authors", "classifiers", "keywords", "license", "version", "urls"]

src/zimscraperlib/rewriting/js.py

@@ -21,6 +21,7 @@
 from collections.abc import Callable, Iterable
 from typing import Any, Literal
 
+from zimscraperlib.rewriting.js_ast import parse_top_level
 from zimscraperlib.rewriting.rx_replacer import (
     RxRewriter,
     TransformationAction,
@@ -348,13 +349,97 @@ def rewrite(self, text: str | bytes, opts: dict[str, Any] | None = None) -> str:
         if opts.get("inline", False):
             new_text = new_text.replace("\n", " ")
 
-        # This is not totally correctly handling globals,
-        # see https://github.com/openzim/python-scraperlib/issues/329
         if wrap_globals:
-            new_text = self.first_buff + new_text + self.last_buff
+            new_text = self._wrap(new_text, GLOBAL_OVERRIDES)
+            if opts.get("inline", False):
+                new_text = new_text.replace("\n", " ")
 
         return new_text
 
+    def _wrap(self, new_text: str, overrides: list[str]) -> str:
+        """Put the script inside the wombat block, and put its globals back.
+
+        The block is a scope, so `const`, `let` and `class` declared at the top
+        level of the script stop being reachable from any other script on the
+        page — which is how a page that declares its data in one <script> and
+        reads it from another comes out broken but silent (#329).
+
+        So the declarations are carried across the block boundary, exactly as
+        wabac.js does it:
+
+          * `let x` is declared before the block and the keyword removed
+            inside it, so the assignment inside writes the outer binding
+          * `const x` and `class X` cannot be split that way, so their value
+            is handed out through `self.___WB_const_x` and re-declared as a
+            const after the block, and the carrier deleted
+          * a name that shadows one of the wombat globals is left alone, and
+            that global is dropped from the wrapper instead
+          * a top-level `document.write()` gets its `document.close()`
+
+        If the script cannot be parsed, none of this happens and the wrapper is
+        exactly what it was before: a script Zimi cannot read is still a script
+        it must not corrupt."""
+        first_buff = self.first_buff
+        last_buff = self.last_buff
+        pre_scope_globals = ""
+        in_scope_globals = ""
+        post_scope_globals = ""
+
+        parsed = parse_top_level(new_text) if new_text else None
+        if parsed is not None:
+            names: list[tuple[str, str]] = []
+            exclude_overrides: set[str] = set()
+            let_offsets: list[int] = []
+            last_start = -1
+            for decl in parsed.declarations:
+                if decl.name in overrides:
+                    exclude_overrides.add(decl.name)
+                    continue
+                if decl.kind == "class":
+                    names.append((decl.name, "const"))
+                elif decl.kind in ("const", "let"):
+                    names.append((decl.name, decl.kind))
+                    if decl.kind == "let" and last_start != decl.start:
+                        let_offsets.insert(0, decl.start)
+                        last_start = decl.start
+
+            if exclude_overrides:
+                first_buff = self._init_local_declaration(
+                    [name for name in overrides if name not in exclude_overrides]
+                )
+            if parsed.has_document_write:
+                last_buff = ";document.close();" + self.last_buff
+
+            # Offsets are byte offsets into the source, and descending, so each
+            # removal leaves the ones still to come valid.
+            data = new_text.encode("utf-8")
+            for offset in let_offsets:
+                data = data[:offset] + data[offset + len("let") :]
+            new_text = data.decode("utf-8", errors="replace")
+
+            for name, kind in names:
+                if kind == "const":
+                    varname = f"self.___WB_const_{name}"
+                    in_scope_globals += f"{varname} = {name};\n"
+                    post_scope_globals += (
+                        f"{kind} {name} = {varname}; delete {varname};\n"
+                    )
+                else:
+                    pre_scope_globals += f"let {name};\n"
+            if in_scope_globals:
+                in_scope_globals = "\n;" + in_scope_globals
+            if post_scope_globals:
+                post_scope_globals = "\n" + post_scope_globals
+
+        return (
+            pre_scope_globals
+            + first_buff
+            + new_text
+            + in_scope_globals
+            + last_buff
+            + post_scope_globals
+        )
+
     def _get_esm_import_rule(self) -> TransformationRule:
         # Capture plain local values instead of closing over `self`: a closure that
         # references `self` here would end up stored in `self.rules`, creating a

src/zimscraperlib/rewriting/js_ast.py

@@ -0,0 +1,125 @@
+"""The little bit of JavaScript parsing the JS rewriter needs.
+
+`js.py` wraps a script in a block so wombat can shadow `window`, `document`
+and friends. A block is a scope, so every top-level `const`, `let` and `class`
+in the script becomes block-scoped too, and stops being visible to any other
+script on the page. wabac.js solves this by parsing the script and hoisting
+those names back out; this module is the parsing half of that, kept behind one
+function so the choice of parser is one import to change.
+
+Only the top level matters. Nothing nested can leak a global, so this never
+walks into a function body, and it answers three questions:
+
+  * which `const`, `let`, `var` and `class` names the script declares at the
+    top level, and of what kind
+  * where each declaration starts, so a `let` keyword can be removed
+  * whether the script calls `document.write()` at the top level
+
+Why tree-sitter and not a pure-Python parser: the scripts this runs on are
+whatever the live web served. `esprima` (the obvious pure-Python choice) is
+ES2017 and refuses optional chaining, class fields and `for await`, all of
+which are ordinary in shipped code today; tree-sitter parses them, and is
+error-tolerant besides, so a script it cannot fully understand still yields
+the declarations it could read rather than an exception.
+"""
+
+from __future__ import annotations
+
+from dataclasses import dataclass
+
+import tree_sitter_javascript
+from tree_sitter import Language, Node, Parser
+
+__all__ = ["Declaration", "TopLevel", "parse_top_level"]
+
+_PARSER = Parser(Language(tree_sitter_javascript.language()))
+
+
+@dataclass(frozen=True)
+class Declaration:
+    """One name a script declares at its top level."""
+
+    name: str
+    kind: str  # "const" | "let" | "var" | "class"
+    start: int  # byte offset of the statement that declares it
+
+
+@dataclass(frozen=True)
+class TopLevel:
+    declarations: list[Declaration]
+    has_document_write: bool
+
+
+def _text(node: Node | None, source: bytes) -> str:
+    """The source a node covers. A missing node reads as no text, so callers
+    can ask for an optional field without a guard at every site."""
+    if node is None:
+        return ""
+    return source[node.start_byte : node.end_byte].decode("utf-8", errors="replace")
+
+
+def _identifiers(node: Node, source: bytes) -> list[str]:
+    """The plain identifiers a declaration binds.
+
+    Destructuring (`const {a, b} = x`) is deliberately skipped, exactly as
+    wabac.js skips anything whose id is not an Identifier: hoisting a
+    destructured binding would mean rebuilding the pattern, and the names it
+    binds are rare enough at the top level of a script to be worth leaving
+    alone rather than getting subtly wrong."""
+    names: list[str] = []
+    for child in node.named_children:
+        name = child.child_by_field_name("name")
+        if name is not None and name.type == "identifier":
+            names.append(_text(name, source))
+    return names
+
+
+def _is_document_write(node: Node, source: bytes) -> bool:
+    if node.type != "expression_statement" or not node.named_children:
+        return False
+    call = node.named_children[0]
+    if call.type != "call_expression":
+        return False
+    callee = call.child_by_field_name("function")
+    if callee is None or callee.type != "member_expression":
+        return False
+    # A member expression always has both fields; anything else is a parser
+    # surprise, and parse_top_level's own net catches those.
+    obj = callee.child_by_field_name("object")
+    prop = callee.child_by_field_name("property")
+    return _text(obj, source) == "document" and _text(prop, source) == "write"
+
+
+def parse_top_level(text: str) -> TopLevel | None:
+    """Read a script's top-level declarations, or None when it cannot be read.
+
+    None means "no opinion", and the caller leaves the script alone — which is
+    what happened to every script before this existed. wabac.js wraps its whole
+    parseGlobals in a try/catch for the same reason, and so does this: nothing
+    here may throw into a scrape."""
+    try:
+        source = text.encode("utf-8")
+        root = _PARSER.parse(source).root_node
+        declarations: list[Declaration] = []
+        has_document_write = False
+        for node in root.named_children:
+            if node.type == "lexical_declaration":
+                # `const` or `let` — `using` has its own node type.
+                kind = _text(node.children[0], source)
+                for name in _identifiers(node, source):
+                    declarations.append(Declaration(name, kind, node.start_byte))
+            elif node.type == "variable_declaration":
+                for name in _identifiers(node, source):
+                    declarations.append(Declaration(name, "var", node.start_byte))
+            elif node.type == "class_declaration":
+                name_node = node.child_by_field_name("name")
+                declarations.append(
+                    Declaration(_text(name_node, source), "class", node.start_byte)
+                )
+            elif not has_document_write and _is_document_write(node, source):
+                has_document_write = True
+        return TopLevel(
+            declarations=declarations, has_document_write=has_document_write
+        )
+    except Exception:
+        return None