html-rewriter: Support streaming content replacements #3211
Conversation
npaun
force-pushed
the
npaun/lol-html-streaming-replacement
branch
from
2ceeba1 to
25d56aa
Compare
src/workerd/api/html-rewriter.h
Outdated
| @@ -304,6 +307,8 @@ class EndTag final: public HTMLRewriter::Token { | |||
|
|
|||
| private: | |||
| kj::Maybe<CType&> impl; | |||
| // TODO(npaun): is adding rewriter field like this legit? | |||
There was a problem hiding this comment.
I would say, not quite legit, because this could become dangling. The reason is that subclasses of Token are the C++-side implementations of JS wrapper objects, meaning JS can arbitrarily extend their lifetimes beyond Rewriter's lifetime. This is the reason there's the TokenScope guard which nullifies impl above.
Of course, checkToken() is called in all of the places we use this rewriter reference, so the code as written is safe, but it feels like leaving a footgun around.
It occurs to me that an alternative could be to make each Token subclass own their own HashMap of registered replacers. We would expect each Token's replacers to be dropped before its associated TokenScope ends, meaning we could have ~TokenScope() assert if there are any registered replacers left in the Token.
There was a problem hiding this comment.
The dangling reference issue is fixed.
However, I didn't move the registered replacers to Token yet because it seems like the replacer callback can be invoked after their registering handler has exited. Happy to revisit this if you have any ideas to try later on.
npaun
force-pushed
the
npaun/lol-html-streaming-replacement
branch
4 times, most recently
from
df2e80f to
9154b3e
Compare
npaun
force-pushed
the
npaun/lol-html-streaming-replacement
branch
from
9154b3e to
64202f3
Compare
npaun
force-pushed
the
npaun/lol-html-streaming-replacement
branch
2 times, most recently
from
62fabff to
dabc3b6
Compare
npaun
force-pushed
the
npaun/lol-html-streaming-replacement
branch
from
dabc3b6 to
da5d326
Compare
npaun
force-pushed
the
npaun/lol-html-streaming-replacement
branch
from
da5d326 to
ce80785
Compare
|
The generated output of |
npaun
force-pushed
the
npaun/lol-html-streaming-replacement
branch
from
d9535a3 to
33c3910
Compare
This reverts commit c77df01, which is causing upstream test failures.
lol-html allows you to insert replacement values in various places in a HTML stream. Previously, these replacement values were always strings. However, recently lol-html added support for streaming replacements in cloudflare/lol-html#229. lol-html requires the streams to produce valid UTF-8 data, but it may be written in arbitrary length chunks (even partial UTF-8 characters).
This PR adds support for replacement values that are
ReadableStreams orResponses.