- 
                Notifications
    You must be signed in to change notification settings 
- Fork 13
feat(@netlify/vite-plugin-react-router): add edge support #562
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
base: main
Are you sure you want to change the base?
feat(@netlify/vite-plugin-react-router): add edge support #562
Conversation
| ✅ Deploy Preview for remix-edge ready!
 To edit notification comments on pull requests, go to your Netlify project configuration. | 
| ✅ Deploy Preview for remix-serverless ready!
 To edit notification comments on pull requests, go to your Netlify project configuration. | 
5b9a00c    to
    b552eb6      
    Compare
  
    744b059    to
    7fde98c      
    Compare
  
    When the globally installed deno cli isn't a supported version, the build automatically tries to install a local binary. Unfortunately when running multiple builds concurrently on the same machine with default configuration, there's a race condition where a binary tries to get written to the same path where one is currently running, leading to an OS error. Just install the correct version globally. See (internal link) https://netlify.slack.com/archives/C03ETTLQ9BP/p1761826639264259
it turns out... > By default, tag-based purges apply to all of the site’s deploys. To target a specific deploy, > specify one or more of the following [...] from https://docs.netlify.com/build/caching/caching-overview/#use-a-function-with-the-purgecache-helper-to-purge-by-cache-tag Whoops. So concurrent tests were conflicting with one another's expectations.
7fde98c    to
    78d1afe      
    Compare
  
    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.
File extracted as-is. Only change is the TNetlifyContext generic, so that NFs and EFs can pass in their own slightly different context types.
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.
Most of this file was extracted as is. The only change is the context generic and the runtimeName arg.
| "./function-handler": { | ||
| "types": "./dist/function-handler.d.mts", | ||
| "default": "./dist/function-handler.mjs" | ||
| }, | ||
| "./edge-function-handler": { | 
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.
Check out the first lines of FUNCTION_HANDLER and EDGE_FUNCTION_HANDLER to understand what these are for. They're the runtime entry points.
(This should probably always have been an explicit separate export, honestly.)
| > required for the Deno runtime. You may customize your server entry file, but see below for important edge runtime | ||
| > constraints. | ||
| ```tsx | 
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.
identical to the Cloudflare one in the RR7 codebase... https://github.com/remix-run/react-router/blob/cb9a090316003988ff367bb2f2d1ef5bd03bd3af/integration/helpers/vite-plugin-cloudflare-template/app/entry.server.tsx
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.
this whole fixture is pretty much identical to the react-router-serverless-site one. I just removed durable, added app/entry.server.tsx, and passed in edge: true and excludedPaths in the vite config.
        
          
                tests/e2e/fixtures/react-router-serverless-v8-middleware/package.json
              
                Outdated
          
            Show resolved
            Hide resolved
        
      When a test retries, it reuses the deployed fixture and since the cache state is preserved from the previous run, the initial state is incorrect. Attaching a unique query string per run isolated each run.
There appears to still be some flakiness that I cannot reproduce when running `.only` on a test, which implies that there's somethhing conflicting somehow across tests running concurrently.
| export type { GetLoadContextFunction, RequestHandler } from './server' | ||
| export { createRequestHandler, netlifyRouterContext } from './server' | ||
| export type { GetLoadContextFunction, RequestHandler } from './function-handler' | ||
| export { createRequestHandler, netlifyRouterContext } from './function-handler' | 
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.
This is a bit confusing to me. We only export serverless context here. But if user opt into using edge: true, the context we will be providing will be this one for edge and not serverless 
| export const netlifyRouterContext = createNetlifyRouterContext<NetlifyEdgeContext>() | 
We don't document that for edge: true case users should be importing from @netlify/vite-plugin-react-router/edge-function-handler to get the "right" context right now
so when users would try to use it as what's documented (and what's added in edge fixture here
remix-compute/tests/e2e/fixtures/react-router-edge-v8-middleware/app/routes/context.tsx
Line 1 in fafc121
| import { netlifyRouterContext } from '@netlify/vite-plugin-react-router' | 
I can see this still ~working (I did not test this, just theory craft given that things generally seems to be working) because of the proxy we did for dev/initial value as far as in fact using EF context (and not getting null / undefined otherwise) (?) I'm not sure if we feel ok relying on this in prod (if this is in fact what happens)
But the types would be potentially incompatible if we leave things as-is?
I do think it makes total sense to preserve this export from main to not generate breaking change for users, but it would seem to me we should document that if you opt into edge - you should import netlifyRouterContext from edge specific module (or add export from main named as netlifyEdgeRouterContext or something like that for ease of use)?
I'm not sure if other exports here are meant to be used by users directly, but if they are - then same thing applies to other exports.
I'm not sure there is any trickery here possible to automate this and automagically give users correct variant of those exports and this seems to me like it might need to be part of docs for edge usage to be correct?
| const entries = await readdir(clientDir, { withFileTypes: true }) | ||
| const excludedPath = [ | ||
| '/.netlify/*', | ||
| ...entries.map((entry) => (entry.isDirectory() ? `/${entry.name}/*` : `/${entry.name}`)), | 
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.
If we only check top-level files and dirs in build/client, then at least in my case it would exclude /assets/* which means users won't be allowed to have their own routes that start with /assets/ segment - given this is in new code path for edge rendering, this at least won't cause regressions, but it might lead to very confusing problems for users opting to use edge rendering
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.
interesting... that seems pretty reasonable to me, honestly 😄. you put assets in assets/ to have them served under assets/... I think doing extra things with that path is playing with fire!
... ah but wait! You're saying actually there's nothing special about the dir assets/. A project could have foo/bar.jpg and we'd exclude all of foo/* from the function. hmm.
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.
Yeah ok, I guess we should enumerate every file in the tree and specifically exclude it 😢.
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.
... ah but wait! You're saying actually there's nothing special about the dir assets/. A project could have foo/bar.jpg and we'd exclude all of foo/* from the function. hmm.
I don't know enough about react-router - I'm not sure if it's possible to inject "random" files to be available (kind of like public directory in Next.js) or wether it would only ever be assets directory in practice?
If it would only ever be assets, then I think it's reasonable enough to accept tradeoff here about not overloading assets path segment by users / playing with fire and to not explode number of excluded paths in configuration that will happen when we need to list all the individual files, but I will leave decision to you here. Just wanted to flag this as potential problem
Summary
This PR adds opt-in support for deploying React Router 7 apps to Netlify Edge Functions (Deno runtime) via the
edge: booleanplugin option. Previously, the plugin only supported Netlify Functions (Node.js runtime).Toward FRB-1519
Changes
edge?: booleanoption to Vite plugin factoryedgeis enabled:target: 'webworker'and other Deno-compatible build config.netlify/v1/edge-functions/, instead of a function.netlify/v1/functions/excludedPaths?: string[]option to allow users to exclude paths from the React Router handler. Required when usingedge: trueand the project also has its own Netlify Functions with a configuredpath, otherwise only the RR handler runs on all dynamic paths.Testing
Documentation
I added complete documentation for opting in (and back out of) edge deployment.
Implementation
Edge support is more involved than regular serverless functions for three main reasons:
preferStaticoption like NFs.excludedPathwith all published client assets. This skips the EF entirely for these paths.path: '/*', if a user has their own NF withpath: '/foo', our EF will run first and render a response from React Router and the user's NF will never run.excludedPaths?: []option to the plugin.edge: truethe user must include their ownapp/entry.server.tsxthat usesrenderToReadableStreaminstead ofrenderToPipeableStream.