Align this.<prop> rewriting with wabac.js's cleaner implementation

CHANGELOG.md

@@ -20,6 +20,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Fixed
 
 - Fix JS rewriting of `this.<prop>` so it reliably detects a leading newline (and prepends the needed `;`) regardless of the amount of whitespace, and no longer wrongly rewrites `this` when preceded by `.`/`$` and several spaces (#381)
+- Align JS rewriting of `this.<prop>` with the cleaner implementation wabac.js landed instead of ours (webrecorder/wabac.js#398, superseding our own webrecorder/wabac.js#395): a leading `this.<prop>` at the very start of a script is now rewritten too, and a preceding `$` only blocks the rewrite when directly adjacent (a `.` still blocks it regardless of the amount of whitespace in between)
 - Patch vendored `wombat.js` so `location.href`/`.search`/`.pathname` (and the rest of the `Location` getters) prefer `wb_info.url` over re-deriving a value via `extractOriginalURL`, fixing their value for any archived page whose captured URL has a query string (#382, webrecorder/wombat#244)
 - Keep escaped Unicode characters in CSS strings instead of turning them into literal characters when rewriting (#377)
 - Patch vendored `wombat.js` so element properties (`img.src`, `script.src`, `link.href`, `a.href` and its derived `.pathname`/`.search`/..., `Attr.value`) return and forget a server-side-rewriter-recorded original the same way `getAttribute` already does (#386, webrecorder/wombat#240)

src/zimscraperlib/rewriting/js.py

@@ -134,36 +134,38 @@ def replace_this_prop() -> TransformationAction:
     """
     Create a rewrite_function replacing "this" by `this_rw`.
 
-    Replacement happens only if "this" is not a property of an object, i.e. if there
-    is a previous token, and it is not "." or "$", once whitespace (including
-    newlines) in between is skipped over. The previous token is looked up in the
-    full original string rather than just the match, because another rule earlier
-    in `create_js_rules` may already have consumed (as part of an unrelated match)
-    whatever directly preceded that whitespace, e.g. the "=" in
-    `const location = this.location` is consumed by the `let/const location =`
-    passthrough rule.
-
-    If that skipped whitespace contains a newline, a semi-colon is also prepended.
-    This guards against a JavaScript Automatic Semicolon Insertion (ASI) pitfall:
-    `this_rw` starts with an identifier immediately followed by `(`, so if it
-    starts a new line right after a statement with no trailing `;`, JS would
-    otherwise parse it as a call of whatever the previous statement evaluated to
-    (e.g. `a = 5\nthis.x = 1` would silently become `a = 5(this.x = 1)`).
+    Replacement happens unless "this" is a property of an object, i.e. unless it is
+    directly preceded by "$" (no whitespace in between), or preceded by "." (any
+    amount of whitespace, including newlines, in between). The start of the script
+    counts as neither, so a leading `this.<prop>` is rewritten.
+
+    The previous token is looked up in the full original string rather than just the
+    match, because another rule earlier in `create_js_rules` may already have
+    consumed (as part of an unrelated match) whatever directly preceded that
+    whitespace, e.g. the "=" in `const location = this.location` is consumed by the
+    `let/const location =` passthrough rule.
+
+    If there is a newline in the whitespace immediately preceding "this", a
+    semi-colon is also prepended. This guards against a JavaScript Automatic
+    Semicolon Insertion (ASI) pitfall: `this_rw` starts with an identifier
+    immediately followed by `(`, so if it starts a new line right after a statement
+    with no trailing `;`, JS would otherwise parse it as a call of whatever the
+    previous statement evaluated to (e.g. `a = 5\nthis.x = 1` would silently become
+    `a = 5(this.x = 1)`).
     """
 
     def f(m_object: re.Match[str], _opts: dict[str, Any] | None) -> str:
         full_string = m_object.string
         pos = m_object.start() - 1
-        has_newline = False
-        while pos >= 0 and full_string[pos].isspace():
-            has_newline = has_newline or full_string[pos] == "\n"
-            pos -= 1
-        if pos < 0 or full_string[pos] in ".$":
-            # No real previous token (start of script) is treated like "." or "$":
-            # kept as-is, consistent with this rule never firing on the very first
-            # token of a script.
-            return m_object[0]
-        if has_newline:
+        ws_start = pos
+        while ws_start >= 0 and full_string[ws_start].isspace():
+            ws_start -= 1
+        if ws_start >= 0:
+            prev_char = full_string[ws_start]
+            is_adjacent = ws_start == pos
+            if prev_char == "." or (prev_char == "$" and is_adjacent):
+                return m_object[0]
+        if "\n" in full_string[ws_start + 1 : pos + 1]:
             return m_object[0].replace("this", ";" + this_rw)
         return m_object[0].replace("this", this_rw)
 

tests/rewriting/test_js_rewriting.py

@@ -31,6 +31,8 @@ def simple_js_rewriter(
         "'a||this||that",
         "(a,b,Q.contains(i[t], this))",
         "a = this.location.href; exports.Foo = Foo; /* export className */",
+        "this.window=red",
+        "aaa$   this.window=red",
     ]
 )
 def rewrite_this_js_content(request: pytest.FixtureRequest):
@@ -57,8 +59,9 @@ def test_this_js_rewrite(
         "aaa.this.window=red",
         "aaa. this.window=red",
         "aaa.   this.window=red",
+        "aaa.\nthis.window=red",
+        "aaa.\n   this.window=red",
         "aaa$this.window=red",
-        "aaa$   this.window=red",
         "a = this.color;",
         "return this.color",
         'func(Function("return this.color"));',
@@ -81,15 +84,24 @@ def test_this_no_js_rewrite(
     )
 
 
+@pytest.mark.parametrize(
+    "prefix",
+    ["aaa", "aaa$"],
+)
 @pytest.mark.parametrize(
     "spaces",
     [0, 1, 2, 3, 4],
 )
-def test_this_js_rewrite_newline(simple_js_rewriter: JsRewriter, spaces: int):
+def test_this_js_rewrite_newline(
+    simple_js_rewriter: JsRewriter, prefix: str, spaces: int
+):
+    # a "$" right before "this" only blocks the rewrite when directly adjacent (no
+    # whitespace in between), so it does not block it here since a newline (and
+    # possibly spaces) is always inserted between "$" and "this"
     padding = " " * spaces
     assert (
-        simple_js_rewriter.rewrite(f"aaa\n{padding}this.window=red")
-        == f"aaa\n{padding};_____WB$wombat$check$this$function_____(this)"
+        simple_js_rewriter.rewrite(f"{prefix}\n{padding}this.window=red")
+        == f"{prefix}\n{padding};_____WB$wombat$check$this$function_____(this)"
         ".window=red"
     )
 
@@ -562,7 +574,6 @@ def test_import_rewrite_force_not_module(simple_js_rewriter: JsRewriter):
         "this.  $location = http://example.com/",
         "this. _location = http://example.com/",
         "this. alocation = http://example.com/",
-        "this.location = http://example.com/",
         ",eval(a)",
         "this.$eval(a)",
         "x = $eval; x(a);",