Carry a script's top-level declarations across the wombat block - #335
Carry a script's top-level declarations across the wombat block#335epheterson wants to merge 3 commits into
Conversation
The rewriter wraps a script in a block so wombat can shadow window,
document and the rest. A block is a scope, so every top-level const, let
and class in that script becomes block-scoped with it and stops being
visible to any other script on the page. The script still runs, so
nothing reports an error; the page is simply wrong.
This ports wabac.js's approach (parseGlobals in src/rewrite/jsrewriter.ts),
keeping its names and its output byte for byte:
* let is declared before the block and its keyword removed inside, so
the assignment writes the outer binding
* const and class are handed out through self.___WB_const_<name> and
re-declared after the block, and the carrier deleted
* a name that shadows one of the wombat globals is left to the script
and that global is dropped from the wrapper
* a top-level document.write() still gets its document.close()
Parsing is behind rewriting/js_ast.py so the parser is one import to
change. 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 silently restores the bug. A script that still cannot be
parsed is wrapped exactly as before rather than corrupted.
Two existing fixtures encoded the old behaviour and are updated. One of
them, a script declaring 'var self', was emitting 'let self' twice in the
same block: a SyntaxError that killed the whole script.
Fixes openzim#329
Adds the remaining cases from wabac.js's test/rewriteJS.ts that exercise this branch: several declarators on one line, let+var+const together, and a carried const alongside a top-level document.write(). The AST walk now sits inside the same try/except as the parse, matching wabac.js, which wraps its whole parseGlobals call. Nothing here may throw into a scrape. Adds the changelog entry.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #335 +/- ##
===========================================
- Coverage 100.00% 99.48% -0.52%
===========================================
Files 41 42 +1
Lines 2572 2693 +121
Branches 366 397 +31
===========================================
+ Hits 2572 2679 +107
- Misses 0 6 +6
- Partials 0 8 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
benoit74
left a comment
benoit74
left a comment
There was a problem hiding this comment.
Looks very promising, thank you.
I like the choice of tree-sitter and it is compatible with our expectations. We do have a separate issue to make library more dynamic in terms of dependencies one which to have installed (#244), so it is clearly outside this PR scope.
Please fix CI: linting QA is failing + code coverage is not at 100% anymore ; regarding code coverage, I'm OK to consider some code does not need to be covered if it is just too painful and/or useless to test, but this obviously needs to be justified.
Linting: imports moved to the top of the file, unused noqa directives dropped, one long line wrapped. tree-sitter is a declared dependency, so the guarded import it had was neither needed nor lintable. Types: pyright strict had no types for the parser because nothing annotated the Node parameters. Annotated, and _text now takes an optional node and reads a missing one as empty, so its callers ask for optional fields without a guard at each site. Coverage is back to 100% with branches on both files. Two of the uncovered branches were dead code rather than untested code, and are gone: a lexical_declaration is only ever const or let (a "using" declaration is its own node type in the grammar), and a member_expression always has both of its fields. parse_top_level's own try/except is the net for any parser surprise, and that net is now tested directly. The rest is covered by four new cases: a destructured declaration is left alone, an inline script stays on one line, a script the parser cannot read is wrapped unchanged, and a parser that raises answers None rather than throwing into a scrape.
|
Both fixed, thanks for the quick look. Linting — imports at the top, unused Coverage — back to 100% with branches on both files, and nothing is excluded. Two of the uncovered branches turned out to be dead code rather than untested code, so they are gone instead of tested: a Types — pyright strict had no types for the parser at all, since nothing annotated the Good to know about #244; happy to move the dependency behind whatever shape that lands on. |
2 participants
Fixes #329.
Ports wabac.js's
parseGlobals(src/rewrite/jsrewriter.ts), keeping its variable names and matching its output byte for byte:let xis pre-declared before the block and its keyword removed inside, so the assignment writes the outer binding.const xandclass Xgo out throughself.___WB_const_xand are re-declared after the block, and the carrier deleted.document.write()still gets itsdocument.close().Parser. Behind
rewriting/js_ast.py, one function, so it is one import to change. I proposed esprima earlier in this issue and then measured it: it is ES2017 and refuses optional chaining, class fields andfor await, all ordinary in shipped code. Since a parse failure falls back to the old behaviour, that would restore the bug on the modern pages most likely to hit it. tree-sitter parses all three, tolerates broken input, and ships wheels. If the dependency is unwelcome, it can be made optional with a fallback, or replaced with a top-level-only tokenizer.Tests.
tests/rewriting/test_js_globals.pyports every case in wabac.js'stest/rewriteJS.tsthat exercises this branch, expected strings kept verbatim: the combined class/const/let/var fixture, several declarators on one line,let+var+const,document.writealone and with a carried const, a shadowed global, nested scopes, and an unparsable script. Plus modern syntax, which wabac's suite predates.Two existing fixtures in
test_js_rewriting.pyencoded the old behaviour and are updated. One is worth flagging:var selfwas emittinglet selftwice in the same block, aSyntaxErrorthat killed the whole script.Suite: 504 passed.
Found on nerdfonts.com/cheat-sheet, which declares
const glyphsin one inline script and reads it from another: the table was in the ZIM, rewritten correctly, and unreachable. I have not run this against a large corpus of real captures; if you have a fixture set for that, point me at it.