feat: add @remix-run/netlify-edge package + update Netlify template
#3104
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,9 @@ | ||
| { | ||
| "imports": { | ||
| "@remix-run/server-runtime": "https://esm.sh/@remix-run/server-runtime@nightly", | ||
| "@remix-run/deno": "../packages/remix-deno/index.ts", | ||
| "@remix-run/deno/": "../packages/remix-deno/", | ||
MichaelDeBoey marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| "@netlify/edge-functions": "https://edge.netlify.com", | ||
|
There was a problem hiding this comment. I think we should add these to the template's There was a problem hiding this comment. No, they're compiled away by then There was a problem hiding this comment. Since We also include it in the template once #3104 (comment) is fixed CC/ @pcattori There was a problem hiding this comment.
There was a problem hiding this comment. Since Deno supports TS, I think we don't need to compile @pcattori can probably give more insights in what's best here. There was a problem hiding this comment. It's imported by code that's then compiled by node and rollup. Using an import map won't make any difference, because rollup will ignore it and just load and bundle There was a problem hiding this comment. I think we don't compile Whatever we decide to do, I think we should streamline it in both the There was a problem hiding this comment. Just trying to see the benefit here. I have it working here, https://github.com/nickytonline/remix/pull/3. Even though our edge function offering uses Deno, the projects people create, at least at the moment are always Node.js projects. We don’t have a deno.json and our tooling is Node.js centric, i,e, the Netlify CLI. As well, they may add Netlify functions for certain things, e,g, scheduled functions, background functions or even a custom function, and these are Node.js based. |
||
| "mime": "https://esm.sh/mime@3.0.0" | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,7 +22,9 @@ export function serverBareModulesPlugin( | |
| remixConfig: RemixConfig, | ||
| onWarning?: (warning: string, key: string) => void | ||
| ): Plugin { | ||
| let isDenoRuntime = ["deno", "netlify-edge"].includes( | ||
|
There was a problem hiding this comment. @jacob-ebey Can you please enlighten me about why this code is here? Why don't we want to use tsconfig to resolve module paths on Deno? And is this applicable to Netlify Edge as well? There was a problem hiding this comment. Deno essentially forks typescript and has their own module resolution algorithm. Using the standard tsconfig won't suffice as it doesn't account for URL imports. When building for deno, we add an esbuild plugin that uses the deno resolution algorithm. There was a problem hiding this comment. Gee, thanks Deno! In that case it sounds like maybe we need another top-level config option (since Does that sound reasonable? |
||
| remixConfig.serverBuildTarget ?? "" | ||
| ); | ||
|
|
||
| // Resolve paths according to tsconfig paths property | ||
| let matchPath = isDenoRuntime | ||
|
|
@@ -108,6 +110,7 @@ export function serverBareModulesPlugin( | |
| case "cloudflare-pages": | ||
| case "cloudflare-workers": | ||
| case "deno": | ||
| case "netlify-edge": | ||
|
There was a problem hiding this comment. Instead of adding this line to our compiler, use |
||
| return undefined; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| # Welcome to Remix! | ||
|
|
||
| [Remix](https://remix.run) is a web framework that helps you build better websites with React. | ||
|
|
||
| To get started, open a new shell and run: | ||
|
|
||
| ```sh | ||
| npx create-remix@latest | ||
| ``` | ||
|
|
||
| Then follow the prompts you see in your terminal. | ||
|
|
||
| For more information about Remix, [visit remix.run](https://remix.run)! |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| import "@remix-run/deno/globals.ts"; | ||
|
|
||
| // @ts-ignore | ||
| globalThis.process ||= { env: Deno.env.toObject() }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| import "./globals.ts"; | ||
|
|
||
| export type { GetLoadContextFunction, RequestHandler } from "./server.ts"; | ||
| export { createRequestHandler } from "./server.ts"; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| { | ||
| "name": "@remix-run/netlify-edge", | ||
| "description": "Netlify Edge server request handler for Remix", | ||
| "version": "1.10.1", | ||
| "license": "MIT", | ||
| "main": "dist/index.js", | ||
| "types": "dist/index.d.ts", | ||
| "repository": { | ||
| "type": "git", | ||
| "url": "https://github.com/remix-run/remix", | ||
| "directory": "packages/remix-netlify-edge" | ||
| }, | ||
| "bugs": { | ||
| "url": "https://github.com/remix-run/remix/issues" | ||
| }, | ||
| "dependencies": { | ||
| "@remix-run/deno": "1.10.1" | ||
| }, | ||
| "devDependencies": { | ||
| "@netlify/edge-functions": "^2.0.0" | ||
nickytonline marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| }, | ||
| "peerDependencies": { | ||
| "@netlify/edge-functions": "^2.0.0" | ||
| }, | ||
| "engines": { | ||
| "node": ">=14" | ||
| }, | ||
| "files": [ | ||
| "dist/", | ||
| "CHANGELOG.md", | ||
| "LICENSE.md", | ||
| "README.md" | ||
| ] | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| const { getAdapterConfig } = require("../../rollup.utils"); | ||
|
|
||
| /** @returns {import("rollup").RollupOptions[]} */ | ||
| module.exports = function rollup() { | ||
| return [getAdapterConfig("netlify-edge")]; | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| import { createRemixRequestHandler } from "@remix-run/deno"; | ||
| import type { AppLoadContext, ServerBuild } from "@remix-run/deno"; | ||
| import type { Context } from "@netlify/edge-functions"; | ||
|
|
||
| type LoadContext = AppLoadContext & Context; | ||
|
|
||
| /** | ||
| * A function that returns the value to use as `context` in route `loader` and | ||
| * `action` functions. | ||
| * | ||
| * You can think of this as an escape hatch that allows you to pass | ||
| * environment/platform-specific values through to your loader/action. | ||
| */ | ||
| export type GetLoadContextFunction = ( | ||
| request: Request, | ||
| context: Context | ||
| ) => Promise<LoadContext> | LoadContext; | ||
|
|
||
| export type RequestHandler = ( | ||
| request: Request, | ||
| context: LoadContext | ||
| ) => Promise<Response | void>; | ||
|
|
||
| export function createRequestHandler({ | ||
| build, | ||
| mode, | ||
| getLoadContext, | ||
| }: { | ||
| build: ServerBuild; | ||
| mode?: string; | ||
| getLoadContext?: GetLoadContextFunction; | ||
| }): RequestHandler { | ||
| let remixHandler = createRemixRequestHandler(build, mode); | ||
|
|
||
| let assetPath = build.assets.url.split("/").slice(0, -1).join("/"); | ||
|
|
||
| return async ( | ||
| request: Request, | ||
| context: LoadContext | ||
| ): Promise<Response | void> => { | ||
| let { pathname } = new URL(request.url); | ||
| // Skip the handler for static files | ||
| if (pathname.startsWith(`${assetPath}/`)) { | ||
| return; | ||
| } | ||
| try { | ||
| let loadContext = (await getLoadContext?.(request, context)) || context; | ||
|
|
||
| let response = await remixHandler(request, loadContext); | ||
| if (response.status === 404) { | ||
|
There was a problem hiding this comment. Why not just include a separate |
||
| // Check if there is a matching static file | ||
| let originResponse = await context.next({ | ||
| sendConditionalRequest: true, | ||
| }); | ||
| if (originResponse.status !== 404) { | ||
| return originResponse; | ||
| } | ||
| } | ||
| return response; | ||
| } catch (error: unknown) { | ||
| console.error(error); | ||
|
|
||
| return new Response("Internal Error", { status: 500 }); | ||
| } | ||
| }; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| { | ||
| "include": ["**/*.ts"], | ||
| "compilerOptions": { | ||
| "lib": ["ES2019", "WebWorker"], | ||
| "target": "ES2019", | ||
| "moduleResolution": "node", | ||
| "allowSyntheticDefaultImports": true, | ||
| "strict": true, | ||
| "declaration": true, | ||
| "emitDeclarationOnly": true, | ||
| "outDir": "../../build/node_modules/@remix-run/netlify-edge", | ||
| "rootDir": ".", | ||
| // Avoid naming conflicts between history and react-router-dom relying on | ||
| // lib.dom.d.ts Window and this being a WebWorker env. | ||
| "skipLibCheck": true | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,5 +2,7 @@ node_modules | |
|
|
||
| /.cache | ||
| /public/build | ||
| .env | ||
|
|
||
| # Local Netlify folder | ||
| .netlify | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,17 +1,14 @@ | ||
| [build] | ||
| command = "remix build" | ||
| publish = "public" | ||
|
|
||
| [[redirects]] | ||
| from = "/*" | ||
| to = "/.netlify/functions/server" | ||
| status = 200 | ||
|
|
||
| [[headers]] | ||
| for = "/build/*" | ||
|
|
||
| [headers.values] | ||
| "Cache-Control" = "public, max-age=31536000, s-maxage=31536000" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should also add it here for consistency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want it here because it's literally only for CI for Netlify Edge. Just the presence of the environment variable is all we care about which is why I didn't include it for the Netlify Functions deploy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our integration tests are also rewritten to be explicit about both cases, so that's why I think we should do it here as well
remix/integration/form-test.ts
Line 468 in 8f7100b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
env vars are strings, so you'd need to change the comparison logic if you do this, because
"false"is truthy.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wouldn't be a problem to do the following check imo