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

feat: support getLoadContext #6

Merged
merged 6 commits into from
Sep 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 12 additions & 7 deletions example/app/routes/_index.tsx
Original file line number Diff line number Diff line change
@@ -1,19 +1,24 @@
import type { LoaderFunctionArgs } from '@remix-run/cloudflare'
import { useLoaderData } from '@remix-run/react'

export const loader = ({ context }: LoaderFunctionArgs) => {
const { env } = context.cloudflare as { env: { MY_VAR: string} }
return {
var: env.MY_VAR
}
export const loader = (args: LoaderFunctionArgs) => {
const extra = args.context.extra
const cloudflare = args.context.cloudflare
return { cloudflare, extra }
}

export default function Index() {
const data = useLoaderData<typeof loader>()
const { cloudflare, extra } = useLoaderData<typeof loader>()
return (
<div>
<h1>Remix and Hono</h1>
<h2>Var is {data.var}</h2>
<h2>Var is {cloudflare.env.MY_VAR}</h2>
<h3>
{cloudflare.cf ? 'cf,' : ''}
{cloudflare.ctx ? 'ctx,' : ''}
{cloudflare.caches ? 'caches are available' : ''}
</h3>
<h4>Extra is {extra}</h4>
</div>
)
}
6 changes: 6 additions & 0 deletions example/e2e.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,12 @@ test('Should return 200 response - /', async ({ page }) => {

const contentH2 = await page.textContent('h2')
expect(contentH2).toBe('Var is My Value')

const contentH3 = await page.textContent('h3')
expect(contentH3).toBe('cf,ctx,caches are available')

const contentH4 = await page.textContent('h4')
expect(contentH4).toBe('Extra is stuff')
})

test('Should return 200 response - /api', async ({ page }) => {
Expand Down
3 changes: 2 additions & 1 deletion example/functions/[[path]].ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// functions/[[path]].ts
import handle from 'hono-remix-adapter/cloudflare-pages'
import { getLoadContext } from 'load-context'
import * as build from '../build/server'
import server from '../server'

export const onRequest = handle(build, server)
export const onRequest = handle(build, server, { getLoadContext })
28 changes: 28 additions & 0 deletions example/load-context.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import type { AppLoadContext } from '@remix-run/cloudflare'
import type { PlatformProxy } from 'wrangler'

interface Env {
MY_VAR: string
}

type Cloudflare = Omit<PlatformProxy<Env>, 'dispose'>

declare module '@remix-run/cloudflare' {
interface AppLoadContext {
cloudflare: Cloudflare
extra: string
}
}

type GetLoadContext = (args: {
request: Request
context: { cloudflare: Cloudflare }
}) => AppLoadContext

// Shared implementation compatible with Vite, Wrangler, and Cloudflare Pages
export const getLoadContext: GetLoadContext = ({ context }) => {
return {
...context,
extra: 'stuff',
}
}
2 changes: 2 additions & 0 deletions example/vite.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
import serverAdapter from 'hono-remix-adapter/vite'
import { defineConfig } from 'vite'
import tsconfigPaths from 'vite-tsconfig-paths'
import { getLoadContext } from './load-context'

export default defineConfig({
plugins: [
Expand All @@ -20,6 +21,7 @@ export default defineConfig({
}),
serverAdapter({
adapter,
getLoadContext,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't leave a comment on line 14, but it should be

    remixCloudflareDevProxy({
      getLoadContext,
    }),

to match the example vite.config.ts from https://remix.run/docs/en/main/guides/vite#augmenting-load-context

I'm not sure if/how that impacts serverAdapter.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I'm not 💯 in my testing, but when I added

    remixCloudflareDevProxy({
      getLoadContext,
    }),

and removed getLoadContext from serverAdapter, the example app still passed my bindings and augmented context through

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About remixCloudflareDevProxy, perhaps it may not need it because this hono-remix-adapter does the same thing. I'll test it; if so, I will create the PR.

entry: 'server/index.ts',
}),
tsconfigPaths(),
Expand Down
41 changes: 32 additions & 9 deletions src/cloudflare-pages.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,38 @@
import type { ServerBuild } from '@remix-run/cloudflare'
import { createRequestHandler } from '@remix-run/cloudflare'
import { Hono } from 'hono'
import { handle } from 'hono/cloudflare-pages'
import { createMiddleware } from 'hono/factory'
import { staticAssets } from 'remix-hono/cloudflare'
import { remix } from 'remix-hono/handler'
import { createGetLoadContextArgs, defaultGetLoadContext } from './remix'
import type { GetLoadContext } from './remix'

export const handler = (serverBuild: any, userApp?: Hono<any, any, any>) => {
interface RemixMiddlewareOptions {
build: ServerBuild
mode?: 'development' | 'production'
getLoadContext: GetLoadContext
}

function remix({ mode, build, getLoadContext }: RemixMiddlewareOptions) {
return createMiddleware(async (c) => {
const requestHandler = createRequestHandler(build, mode)
const args = createGetLoadContextArgs(c)

const loadContext = getLoadContext(args)
return await requestHandler(
c.req.raw,
loadContext instanceof Promise ? await loadContext : loadContext
)
})
}

type Options = {
getLoadContext: GetLoadContext
}

// Relaxing the type definitions
// eslint-disable-next-line @typescript-eslint/no-explicit-any
export const handler = (serverBuild: any, userApp?: Hono<any, any, any>, options?: Options) => {
const app = new Hono()

if (userApp) {
Expand All @@ -18,13 +47,7 @@ export const handler = (serverBuild: any, userApp?: Hono<any, any, any>) => {
return remix({
build: serverBuild,
mode: 'production',
getLoadContext(c) {
return {
cloudflare: {
env: c.env,
},
}
},
getLoadContext: options?.getLoadContext ?? defaultGetLoadContext,
})(c, next)
}
)
Expand Down
20 changes: 13 additions & 7 deletions src/dev.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
import type { AppLoadContext } from '@remix-run/cloudflare'
import { Hono } from 'hono'
import { createGetLoadContextArgs, defaultGetLoadContext } from './remix'
import type { GetLoadContext } from './remix'

export const handle = (userApp?: Hono) => {
type Options = {
getLoadContext: GetLoadContext
}

export const handle = (userApp?: Hono, options?: Options) => {
const app = new Hono()

if (userApp) {
Expand All @@ -13,13 +18,14 @@ export const handle = (userApp?: Hono) => {
const build = await import('virtual:remix/server-build')
const { createRequestHandler } = await import('@remix-run/cloudflare')
const handler = createRequestHandler(build, 'development')
const remixContext = {
cloudflare: {
env: c.env,
},
} as unknown as AppLoadContext

const getLoadContext = options?.getLoadContext ?? defaultGetLoadContext
const args = createGetLoadContextArgs(c)

const remixContext = getLoadContext(args)
return handler(c.req.raw, remixContext)
})

return app
}

Expand Down
34 changes: 34 additions & 0 deletions src/remix.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import type { AppLoadContext } from '@remix-run/cloudflare'
import type { Context } from 'hono'

export type GetLoadContext = (args: {
request: Request
context: {
// Relaxing the type definition
// eslint-disable-next-line @typescript-eslint/no-explicit-any
cloudflare: any
Comment on lines +7 to +9
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like we could do more than any here.

It might be tricky/impractical to use the "real" Env and PlatformProxy types, but I think this works

Suggested change
// Relaxing the type definition
// eslint-disable-next-line @typescript-eslint/no-explicit-any
cloudflare: any
cloudflare: {
env: Record<string, unknown>
cf: IncomingRequestCfProperties
ctx: ExecutionContext
caches: typeof caches
}

}
}) => AppLoadContext

// eslint-disable-next-line @typescript-eslint/no-explicit-any
export const defaultGetLoadContext = ({ context }: any) => {
Comment on lines +13 to +14
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Names are hard, but if you gave that type a name like:

type SomeContext = {
  cloudflare: {
    env: Record<string, unknown>
    cf: IncomingRequestCfProperties
    ctx: ExecutionContext
    caches: typeof caches
  }
};

We could use it like

export type GetLoadContext = (args: {
  request: Request
  context: SomeContext
}) => AppLoadContext

export const defaultGetLoadContext = ({ context }: { context: SomeContext}) => {

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you override the definition of AppLoadContext in your app: https://github.com/yusukebe/hono-remix-adapter/pull/6/files#diff-fe3b5413a3ab56944fc97a3eb9b71e83c41e4d0b41a2303932ae92bd8c82cc8fR11-R13

The type mismatch will occur. To remove the typo error, I've added the any. This may be fixed in the future, but it remains as any.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yusukebe Oh, right. Apologies for not testing that locally. I think 0.2.0 shipped with this type, because I'm getting this type error in my app after updating

Nothing a @ts-expect-error can't fix, though

Screenshot 2024-09-23 at 7 46 38 AM Screenshot 2024-09-23 at 7 40 38 AM

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jfsiii

Oooops. Sorry. The building way was wrong. Fixed it and released the new version. Try v0.2.1.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries. It happens. And it was kind of my fault, anyway.

Thanks!

return {
...context,
}
}

export const createGetLoadContextArgs = (c: Context) => {
return {
context: {
cloudflare: {
env: c.env,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure about this one. Do we want c.env which will return

{
      "MY_VAR": "My Valuez",
      "ASSETS": {},
      "eventContext": {
        "request": {},
        "functionPath": "/",
        "params": {},
        "data": {},
        "env": {
          "MY_VAR": "My Valuez",
          "ASSETS": {}
        }
      }
}

or c.env.eventContext.env, which would only return

        {
          "MY_VAR": "My Valuez",
          "ASSETS": {}
        }

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is also used in the dev server: https://github.com/yusukebe/hono-remix-adapter/pull/6/files#diff-34d76a96faa930c4b9909c055beb422e601e2267f72ce12d6f15a1c2cc6ea914R23

It should be c.env to get MY_VAR correctly since it does not have eventContext.

cf: c.req.raw.cf,
ctx: {
...c.executionCtx,
},
caches,
},
},
request: c.req.raw,
}
}
6 changes: 5 additions & 1 deletion src/vite-plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import type { Plugin } from 'vite'
import fs from 'node:fs'
import path from 'node:path'
import { fileURLToPath } from 'node:url'
import type { GetLoadContext } from './remix'

interface Adapter {
env?: Record<string, unknown> | Promise<Record<string, unknown>>
Expand All @@ -17,6 +18,7 @@ interface Adapter {
interface Options {
entry: string
adapter?: () => Adapter | Promise<Adapter>
getLoadContext?: GetLoadContext
}

export default (options: Options): Plugin => {
Expand Down Expand Up @@ -51,7 +53,9 @@ export default (options: Options): Plugin => {
}

const devModule = await server.ssrLoadModule(devPath)
return devModule['default'](app)
return devModule['default'](app, {
getLoadContext: options.getLoadContext,
})
},
})
}