Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🐛 Support HTMLRewriter replacing HTML with content from a ReadableStream or Response #2758

Open
1000hz opened this issue Sep 20, 2024 · 3 comments
Labels
api feature request Request for Workers team to add a feature

Comments

@1000hz
Copy link
Contributor

1000hz commented Sep 20, 2024

Currently, HTMLRewriter only supports replacing HTML content with strings:

kj::String unwrapContent(Content content) {
return kj::mv(JSG_REQUIRE_NONNULL(content.tryGet<kj::String>(), TypeError,
"Replacing HTML content using a ReadableStream or Response object is not "
"implemented. You must provide a string."));
}

Being able to accept content via a ReadableStream or Response would be very useful for improving the latency of the response to the end user when (for example) embedding content from an upstream fetch into another HTML document.

This depends on first adding underlying support for this in https://github.com/cloudflare/lol-html which will then need to be exposed in the runtime.

@jasnell jasnell added feature request Request for Workers team to add a feature api labels Sep 20, 2024
@IgorMinar
Copy link
Collaborator

Message from @dotjs from this morning:

We discussed this morning and there is some work from the Rust perspective. More problematic is how to integrate with kj. We need their streams to work with Rust streams and we would need some way to make async Rust work with kj-runtime. So I think we would need some runtime effort as well.

That's how we landed on this issue on workerd.

@jasnell @anonrig I'm not a kj expert but I know you have a lot of experience. Do you understand what Andrew's message above means and how much work it would be for us to fix it in the runtime? Is that something you would be comfortable doing or do we need someone else from the runtime.

As for priority, this is become a blocking issue for wider Fractus rollout because the lack of this feature means we can't support streaming SSR, which is a big performance problem for us.

@jasnell
Copy link
Member

jasnell commented Sep 20, 2024

It's been a while since I've dug through this code, would need some time to analyze it again.

@IgorMinar
Copy link
Collaborator

Could you take a peek please? We need to get unblocked soon (within the next few days), and if this path will take longer than that, then we'll most likely reach for an less performant alternative that supports streaming which in the end is going to be on overall perf boost for us even if the transformation itself is inefficient.

Our current top alternative is something like https://trumpet.gofunky.fun/ which seems fine, but we'd have to turn on the nodejs_compat layer which further complicates things but might be worth it if we get unblocked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api feature request Request for Workers team to add a feature
Projects
None yet
Development

No branches or pull requests

3 participants