jrf0110 / 8track Goto Github PK

Since the HTMLRewriter API's .transform(res) returns a new Response instance, this makes working with middleware difficult. I suggest a new ctx method which collects all transforms until the response is handled:

// Transform all HTML responses at /foo such that:
// <h1>Bar</h1><h2>Baz</h2>
app.all`/foo`
  .use((ctx, next) => {
    ctx.transform().on('h1', { text: node => node.replace('Bar') })
    return next()
  })
  .use((ctx, next) => {
    ctx.transform().on('h2', { text: node => node.replace('Baz') })
    return next()
  })
  .handle(ctx => ctx.html('<h1>Foo</h1><h2>Bar</h2>'))

The transforms here don't work like streams piping into one-another, but rather like a single transform instance.

It shouldn't matter where the Response came from, the edge cache middleware should be able to cache it and serve it early in the middleware chain.

// These would be our cache middleware options
type CacheOptions = {
  getCacheKey?: (req: URL) => string
  cache?: typeof caches.default
  edgeTTL?: number
  browserTTL?: number
  edgeTTLForHTML?: number
  browserTTLForHTML?: number
  allowedMethods?: Method[] 
}

import { Router, cache } from '8track'

const app = new Router()

// Cache all GET requests
app.get`(.*)`.use(cache())
...

The changes introduced by #23 and related PRs are not backward compatible and should have resulted in a major version bump; which would have prevented npm/yarn from upgrading the dependency on 8track and breaking our application.

Could you please consider using semver so that breaking changes are properly identified in the future?

Module-based scripts don't receive a FetchEvent, rather they get a request and other assorted info.

https://developers.cloudflare.com/workers/cli-wrangler/configuration#modules

That makes it harder to use with 8track. So far we've been working with

      const e = ({
        request,
      } as any) as FetchEvent

but that's not great as it steps around type safety.

It'd be great if there was something like getResponseForEvent that took the request only.

Due to this change https://developers.cloudflare.com/workers/platform/compatibility-dates#settersgetters-on-api-object-prototypes Response merging in 8track has issues. The way around it is to enable the negation flag for that change

Advised to get to at least version 0.21.1
See https://www.npmjs.com/advisories/1594

Right now, the glue code required for consumers is pretty significant:

import { getErrorPageHTML } from '8track';
import { router } from './router';

addEventListener('fetch', event => {
  const res = router.getResponseForEvent(event).catch(error => {
    return new Response(getErrorPageHTML(event.request, error), {
      status: 500,
      headers: {
        'Content-Type': 'text/html',
      },
    });
  });

  event.respondWith(res as any);
});

Edit

I previously thought this was only the case for ctx.end(new Response()). However, this applies to any afterware effects

For instance:

app.get(`(.*)`).use(async (ctx, next) => {
  await next()
  ctx.response.headers.append('Hello', 'World')
}).handle(async ctx => ctx.end(new Response('Hi'))

The 'Hello: World' Header will not make it into the final response. This is due to ctx being a different instance from middleware to middleware.

I originally had this ticket slated for a V2 release, but I think it's buggy enough to make it into 1.x as well.

Original

Suppose you have some middleware which adds a header and some route handler which calls .end() with a new Response::

app.get(`(.*)`).use(async (ctx, next) => {
  ctx.response.headers.append('Hello', 'World')
  return next()
}).handle(async ctx => ctx.end(new Response('Hi'))

This is confusing but not entirely surprising since the new Response will override the old response and not get any headers. There should be a way for middlewares and handlers to compose nicely and have responses merged together.

8track behaves a bit differently than koa in that the response is not sent until after all middleware chains have unwound. This should be fixed in #42.

Until then, one could add the header appending after an await next(), but this confusing to me - Why should the response be editable after that point? Similarly, HTMLRewriter becomes unviable after response becomes immutable after await next().

I propose we do a couple of things to make this nicer:

1. Add a new property headers on ctx which will collect headers in a context and merge them with the final response
2. Add a new method .transform() which accepts an HTMLRewriter instance. Transforms will be collected and merged with the final response

Hey jrf0110,

I built a similar framework, perhaps it can serve as an inspiration. It also features typed (optional) path parameters, but it does it without the need for literal strings (perhaps this is on your to-do list too?). Here's a GIF

It also allows you to type the state (what you call ctx.data) a handler/middleware expects and will throw a type error if you get it wrong, I think that's possible here too? That part is not super elegant though..

Check it the intro post here, curious to hear your thoughts!