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

Router doesn't route when using service bindings #182

Closed
vladinator1000 opened this issue Apr 29, 2024 · 7 comments
Closed

Router doesn't route when using service bindings #182

vladinator1000 opened this issue Apr 29, 2024 · 7 comments

Comments

@vladinator1000
Copy link

vladinator1000 commented Apr 29, 2024

Hello there, I have a wee bug report. On branch next, it seems like the router fails when when using the service bindings Cloudflare API.

Steps:

  1. Clone this reproduction repo https://github.com/vladinator1000/worktop-service-bindings/tree/main
  2. cd into the api folder
  3. npm i && npm start
  4. Open another terminal
  5. cd into the pages folder
  6. npm i && npm start
  7. In your browser, open http://localhost:9787/api
  8. You should see "Hello from Worktop!" but instead there's "Not found"
@vladinator1000
Copy link
Author

vladinator1000 commented Apr 30, 2024

Hi @lukeed, after some digging, it looks like the issue looks related service bindings. I've updated the issue description to include a full reproduction.

@vladinator1000 vladinator1000 changed the title router.onerror doesn't include the error Router doesn't route when using service bindings Apr 30, 2024
@vladinator1000
Copy link
Author

Oh crap I realized the URL hostname must match exactly for this to work...

In Pages I'm doing

    if (url.pathname.startsWith('/api')) {
      return env.API.fetch(request)
    }

But in the API I was doing

let router = new Router();

router.add('GET', '/', (_request, context) => {
	return new Response('Hello from Worktop!');
});

It didn't find an /api route so it returned "Not Found"...
Fixed it by changing the route like this

router.add('GET', '/api', handler)

I feel so stupid 😂

@lukeed
Copy link
Owner

lukeed commented Apr 30, 2024

All good! It happens :)

PS I would suggest using router.mount so that you can omit the /api/ prefix inside all your API route definitions.

Also, I'm personally not a fan of service bindings. They're just an extra abstraction that makes you think about separation but is never actually separate. SO if I were you, I'd add a pre-build script inside the Pages project that produces the _worker.js file... I'm not familiar with Pages latest build rules/automations, so if they pick up a _worker.ts automatically, then great

@vladinator1000
Copy link
Author

vladinator1000 commented Apr 30, 2024

I agree... The only reason I'm using service bindings is because it runs faster because the bound worker runs on the same thread and bypasses the network.

The Pages advanced mode expects a "_worker.js" in the build directory, which I build in CI with this script:

import { $ } from 'zx'

let branch = process.env.CF_PAGES_BRANCH

let mode = branch === 'main' ? 'production' : 'staging'
await $`yarn vite build --mode ${mode} --ssr src/_worker.ts --emptyOutDir false`.pipe(
  process.stdout,
)

The _worker.ts ended up looking like this

// This is the entry point of the server
// https://developers.cloudflare.com/pages/functions/advanced-mode/
interface Env {
  ASSETS: Fetcher
  API: Fetcher
}

let handler: ExportedHandler<Env> = {
  async fetch(request, env): Promise<Response> {
    const url = new URL(request.url)

    if (url.pathname.startsWith('/api')) {
      url.pathname = url.pathname.replace('/api', '') // avoid adding the "/api" path on the other API worker

      let apiRequest = new Request(url, request)

      return env.API.fetch(apiRequest)
    }

    return env.ASSETS.fetch(request)
  },
}

export default handler

Honestly I want to have a monorepo where the API is part of the pages worker, but my project is multi-repo, started before Pages was a thing.

@lukeed
Copy link
Owner

lukeed commented Apr 30, 2024

My suggestion is to bundle the API code directly within your pages/_worker.js -- you get all the same performance benefits (but more, actually) since it's literally the same Worker/isolate w/o any fake/feigned separation between them

But ya, you might have other repo/Pages challenges to deal with first. Manual deployments will help you out a lot... relying on Pages build pipeline to react to new commits gets old fast (and really is what limits monorepos)

@vladinator1000
Copy link
Author

@lukeed don't tempt me... I'm afraid of getting lost in a monorepo tooling binge 😅

@lukeed
Copy link
Owner

lukeed commented May 1, 2024

Come on in, water's warm

Image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants