feat: add @remix-run/netlify-edge package + update Netlify template
#3104
Conversation
|
Hi @nickytonline, Welcome, and thank you for contributing to Remix! Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once. You may review the CLA and sign it by adding your name to contributors.yml. Once the CLA is signed, the If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at hello@remix.run. Thanks! - The Remix team |
|
Thank you for signing the Contributor License Agreement. Let's get this merged! π₯³ |
MichaelDeBoey
changed the title
@remix-run/netlify-edge package + update Netlify template
There was a problem hiding this comment.
Really awesome PR @nickytonline! π₯³π
Just a couple of remarks to streamline the new adapter package with all existing ones.
|
So far, what's in the PR is the reverted commits @mcansh mentioned in #3092 that from the experimental-netlify-edge branch and I merged latest of the |
|
@nickytonline No hurry, just wanted to make sure this new adapter is aligned with all existing ones. I think it would also be a good idea to add tests to it as well. |
|
|
|
Just as a heads up, @jacob-ebey found a way to get Netlify Edge working purely through the template and without modifying the compiler. // remix.config.js
/**
* @type {import('@remix-run/dev').AppConfig}
*/
module.exports = {
server: "./server.js",
ignoredRouteFiles: [".*"],
serverBuildTarget: "cloudflare-pages",
serverBuildPath: ".netlify/edge-functions/server.js",
serverDependenciesToBundle: [/.*/],
};and in {
"dependencies": {
"@netlify/functions": "latest",
"@remix-run/netlify-edge": "experimental-netlify-edge",
"@remix-run/react": "^1.4.3",
"@remix-run/server-runtime": "^1.4.3",
"cross-env": "^7.0.3",
"isbot": "^3.4.6",
"react": "^18.0.0",
"react-dom": "^18.0.0"
},
"devDependencies": {
"@remix-run/dev": "^1.4.3",
"@remix-run/eslint-config": "^1.4.3",
"@types/react": "^18.0.5",
"@types/react-dom": "^18.0.1",
"eslint": "^8.11.0",
"typescript": "^4.5.5"
},
}So we should stop efforts to modify the Remix compiler for Netlify Edge support and focus on doing so via templates instead. |
|
I'm still testing things out locally. |
|
I'm going to go with @mcansh's suggestion for testing this out locally as it's a bit of a unique situation as we're not just testing out the template.
- ["dependencies", "devDependencies"].forEach((pkgKey) => {
- for (let dependency in appPkg[pkgKey]) {
- let version = appPkg[pkgKey][dependency];
- if (version === "*") {
- appPkg[pkgKey][dependency] = semver.prerelease(remixVersion)
- ? // Templates created from prereleases should pin to a specific version
- remixVersion
- : "^" + remixVersion;
- }
- }
- });
+ ["dependencies", "devDependencies"].forEach((pkgKey) => {
+ for (let dependency in appPkg[pkgKey]) {
+ let version = appPkg[pkgKey][dependency];
+ if (version === "*") {
+ if (process.env.REMIX_USE_BUILD_DEPS) {
+ appPkg[pkgKey][dependency] = `file:${path.relative(
+ projectDir,
+ process.cwd()
+ )}`;
+ } else {
+ appPkg[pkgKey][dependency] = semver.prerelease(remixVersion)
+ ? // Templates created from prereleases should pin to a specific version
+ remixVersion
+ : "^" + remixVersion;
+ }
+ }
+ }
+ });
This is required for the time being because just running the template won't be enough as it'll crash on a missing dep version during npm install. |
Just following up on this @pcattori as there were some discussions on Discord. Based on what I read in Discord, this makes sense, but I think @ascorbic makes a good point about naming the target with a more generic name like If that's the consensus, I'll go ahead and close this PR and open another PR that integrates the Netlify Edge functions template changes only from https://github.com/netlify/remix-edge-template Anything to add @ascorbic? |
|
@nickytonline I think it would be a good idea to wait for #3117 to be merged before continuing here, as that will impact this PR a lot. |
|
@nickytonline #3117 is now merged, so I think it would be a good idea to rebase this PR onto latest We can see what needs to be done from that point on. |
I had rebased onto the branch earlier, but now that it's merged into dev, I'll rebase back onto dev. |
|
Thanks for all the great feedback @MichaelDeBoey! It's much appreciated as I'm still pretty new to Remix. I'm back on this today. π |
|
@nickytonline If you need any help, I'm happy to help out. |
| ".netlify/edge-functions/" | ||
| ], | ||
| "deno.unstable": true, | ||
| "deno.importMap": ".netlify/edge-functions-import-map.json", |
There was a problem hiding this comment.
This file doesn't seem to exist?
We should probably have a .vscode folder instead (like we have in deno template) and copy over the whole folder if people choose for Netlify Edge
There was a problem hiding this comment.
remix.init generates a .vscode folder for this. Also, you mentioned it elsewhere, but we don't need the import map like the deno server build target. We only need our import map because it's only for Netlify Edge functions.
const files = [
...
["vscode.json", join(".vscode", "settings.json")],
...
];
async function main({ rootDirectory }) {
if (await shouldUseEdge()) {
await fs.mkdir(join(rootDirectory, ".vscode"));
for (let [file, target] of files) {
await fs.copyFile(
join(rootDirectory, "remix.init", file),
join(rootDirectory, target || file)
);
}
}
}There was a problem hiding this comment.
I understand vscode.json is copied to .vscode/settings.json when people choose for Netlify Edge in remix.init & that's how I would expect it to be.
However, it is referencing edge-functions-import-map.json, which doesn't exist in the template.
I would suggest to create remix.init/vscode with a settings.json & resolve_npm_imports.json file in it & copy the whole folder to .vscode when people choose for Netlify Edge.
There was a problem hiding this comment.
I forgot to mention edge-functions-import-map.json is generated by us once they've set up the template and build the first time.
I'm probably not explaining it well but we don't need the imports that the deno server build target has for the VS Code deno extension.
There was a problem hiding this comment.
Will it also include all used dependencies, like we have in the deno template?
"imports": {
"@remix-run/deno": "https://esm.sh/@remix-run/deno@1.5.0",
"@remix-run/dev/server-build": "https://esm.sh/@remix-run/dev@1.5.0/server-build",
"@remix-run/react": "https://esm.sh/@remix-run/react@1.5.0",
"react": "https://esm.sh/react@17.0.2",
"react-dom": "https://esm.sh/react-dom@17.0.2",
"react-dom/server": "https://esm.sh/react-dom@17.0.2/server"
}There was a problem hiding this comment.
@ascorbic So if I understand correctly, the template should look/work like a Node template, except using @remix-run/deno instead of @remix-run/node?
There was a problem hiding this comment.
Pretty much, yes. It's the emitted code that needs to be Deno-compatible, not the source.
There was a problem hiding this comment.
@ascorbic But people can write Deno source code?
Or is only Node source code accepted?
There was a problem hiding this comment.
You can do either. Remix compiles everything with Node, so it needs to be acceptable to rollup, but it passes URL imports through unchanged so you can use those too and Deno will use them. You can use Deno globals too.
There was a problem hiding this comment.
Then I think we should treat this template as the Deno template, which means we don't need a tsconfig.json & we need everything we have in the Deno template in .vscode/resolve_npm_imports.json
CC/ @pcattori
|
I merged latest and when I run the generated playground for the Netlify Edge template or Netlify Functions template, I get the following error. Is this a known issue in dev or am I missing something aside from merging latest from the upstream dev branch? I've also bumped the version to 1.9.0 in the remix-netlify-edge package.json for the package itself as well as |
|
@nickytonline CI is passing, so if you have this locally, maybe you should remove |
I had done a |
|
I also noticed that the |
@chaance Maybe this went wrong somehow when merging? |
had to remove it as it prevented experimental and nightly releases from going through due to deno trying to pull in the new version before it was published - i plan on looking into it more soon |
β¦TS errors from node_modules"
β¦pdate-netlify-remix-template
β¦pdate-netlify-remix-template
|
Just giving an update here @MichaelDeBoey. Everything is working. Not sure why I still have issues locally even though CI passes, but that's probably a me problem. I believe everything/most things have been addressed? I've removed the compiler specific code for Netlify Edge in favour of our insource config now. See I know there are plans to pull out server build target for v2, https://youtu.be/D1YiWqWygz4?list=PLXoynULbYuEDR_w8LJKRIkGheWpFfiMiR&t=934, but for the time being I've gone with the same convention as other server build targets for Netlify Edge. I think @ascorbic's proposal, #4878, is a great direction for that. I know there is still the putting back deno type checking, #3104 (comment), but aside from that let me know if there is anything else that has been omitted. I'm off until January, but looking forward to getting this in 2023! π |
|
Hi @MichaelDeBoey, just following up here as I'm back from PTO now. |
| @@ -1,7 +1,7 @@ | |||
| # Welcome to Remix! | |||
There was a problem hiding this comment.
This template should be a repo somewhere on the Netlify GH org. This templates dir is a temporary spot for legacy v1 templates. No new templates should be created here.
| @@ -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 serverDependenciesToBundle in your template config to automatically bundle everything.
| @@ -22,7 +22,9 @@ export function serverBareModulesPlugin( | |||
| remixConfig: RemixConfig, | |||
| onWarning?: (warning: string, key: string) => void | |||
| ): Plugin { | |||
| let isDenoRuntime = remixConfig.serverBuildTarget === "deno"; | |||
| 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 serverBuildTarget is going away) that controls how we resolve modules... maybe something like:
moduleResolution: "node" | "tsconfig" | "deno"
Does that sound reasonable?
| 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 createRequestHandlerWithStaticFiles like we do in our deno adapter?
|
Thanks for all the feedback in this PR. I'm going to go ahead and close this and continue the work in a Netlify repo with the adapter and server runtime as discussed (https://github.com/netlify/remix-compute). |
Description
Adds Edge support for Netlify.
This brings in the work that currently resides in the
experimental-netlify-edgebranch and updates the Netlify template for Remix with the more up to date Edge configuration/files found in https://github.com/netlify/remix-edge-template.It also introduces the
@remix-run/netlify-edgepackage to Remix.Relates to:
Update the Netlify Template in the RepositoryΒ #3092
deprecate this projectΒ netlify/remix-template#71
Docs
Tests (added deployment test)
Testing Strategy
Getting Started
gh co 3104yarnto install all the dependencies of the projecttemplates/netlifyto a new folder you need to create calledscripts/playground/template.local. This is required so that we can run the Netlify template in the Remix testing playground. (see https://github.com/remix-run/remix/blob/main/docs/pages/contributing.md#playground)yarn watch. You'll see some output like this:npm run playground:newto start the playground. You'll see output in the terminal similar to this:Test Remix Running on Netlify Functions
playground-*, e.g../playground/playground-1669172891010yarn watchthat is currently running is handling the dependencies fornode_moduleschmod 777 node_modules/.bin/remix(for some reason in the playground you need to do this, at least on my machine)netlify build --offlineto build the project.netlify dev --framework=#staticTest Remix Running on Netlify Edge Functions
playground-*, e.g../playground/playground-1669172891010yarn watchthat is currently running is handling the dependencies fornode_moduleschmod 777 node_modules/.bin/remix(for some reason in the playground you need to do this, at least on my machine)netlify build --offlineto build the project.netlify dev --framework=#static