remix icon indicating copy to clipboard operation
remix copied to clipboard

feat: add `@remix-run/netlify-edge` package + update Netlify template

Open nickytonline opened this issue 2 years ago β€’ 55 comments

Description

Adds Edge support for Netlify.

This brings in the work that currently resides in the experimental-netlify-edge branch 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-edge package to Remix.

Relates to:

  • #3092

  • https://github.com/netlify/remix-edge-template/issues/71

  • [ ] Docs

  • [x] Tests (added deployment test)

Testing Strategy

Getting Started

  1. Checkout this branch, e.g. via the GitHub CLI gh co 3104
  2. Ensure that the Netlify CLI is installed or up to date.
  3. Run yarn to install all the dependencies of the project
  4. Copy all the files in templates/netlify to 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)
  5. Run yarn watch. You'll see some output like this:
rollup v2.75.7
bundles packages/remix-netlify-edge/index.ts β†’ build/node_modules/@remix-run/netlify-edge/dist...
created build/node_modules/@remix-run/netlify-edge/dist in 24ms
watch.onEnd $ node scripts/copy-build-to-dist.mjs
  πŸ›   Copying build/node_modules/@remix-run/architect/dist to packages/remix-architect/dist
  πŸ›   Copying build/node_modules/@remix-run/cloudflare/dist to packages/remix-cloudflare/dist
  πŸ›   Copying build/node_modules/@remix-run/cloudflare-pages/dist to packages/remix-cloudflare-pages/dist
  πŸ›   Copying build/node_modules/@remix-run/cloudflare-workers/dist to packages/remix-cloudflare-workers/dist
  πŸ›   Copying build/node_modules/@remix-run/dev/dist to packages/remix-dev/dist
  πŸ›   Copying build/node_modules/@remix-run/express/dist to packages/remix-express/dist
  πŸ›   Copying build/node_modules/@remix-run/netlify/dist to packages/remix-netlify/dist
  πŸ›   Copying build/node_modules/@remix-run/netlify-edge/dist to packages/remix-netlify-edge/dist
  πŸ›   Copying build/node_modules/@remix-run/node/dist to packages/remix-node/dist
  πŸ›   Copying build/node_modules/@remix-run/react/dist to packages/remix-react/dist
  πŸ›   Copying build/node_modules/@remix-run/serve/dist to packages/remix-serve/dist
  πŸ›   Copying build/node_modules/@remix-run/server-runtime/dist to packages/remix-server-runtime/dist
  πŸ›   Copying build/node_modules/@remix-run/vercel/dist to packages/remix-vercel/dist
  πŸ›   Copying build/node_modules/create-remix/dist to packages/create-remix/dist
  πŸ›   Copying build/node_modules/remix/dist to packages/remix/dist
  πŸ›   Writing globals.d.ts shim to build/node_modules/@remix-run/node/globals.d.ts
  πŸ›   Copying build/node_modules/@remix-run/dev/server-build.js to packages/remix-dev/server-build.js
  πŸ›   Copying build/node_modules/@remix-run/dev/dist/server-build.d.ts to build/node_modules/@remix-run/dev/server-build.d.ts
  πŸ›   Copying build/node_modules/@remix-run/dev/dist/server-build.d.ts to packages/remix-dev/server-build.d.ts
  πŸ›   Copying build/node_modules/@remix-run/node/globals.d.ts to packages/remix-node/globals.d.ts
  βœ… Successfully copied build files to package dist directories!

[2022-11-22 17:33:29] waiting for changes...
  1. In another terminal window from the root of the Remix project, run npm run playground:new to start the playground. You'll see output in the terminal similar to this:
➜ yarn playground:new
yarn run v1.22.19
$ node ./scripts/playground/new.js
ℹ️  Using local template: /Users/nicktaylor/dev/work/remix/scripts/playground/template.local
πŸ“₯  Installing deps...
npm WARN deprecated @npmcli/[email protected]: This functionality has been moved to @npmcli/fs
npm WARN deprecated [email protected]: See https://github.com/lydell/source-map-url#deprecated
npm WARN deprecated [email protected]: Please see https://github.com/lydell/urix#deprecated
npm WARN deprecated [email protected]: https://github.com/lydell/resolve-url#deprecated
npm WARN deprecated [email protected]: This package has been deprecated and is no longer maintained. Please use @rollup/plugin-inject.
npm WARN deprecated [email protected]: See https://github.com/lydell/source-map-resolve#deprecated

added 1051 packages, and audited 1052 packages in 7s

226 packages are looking for funding
  run `npm fund` for details

found 0 vulnerabilities
πŸ—  Building remix...
$ /Users/nicktaylor/dev/work/remix/node_modules/.bin/rollup -c

packages/create-remix/cli.ts β†’ build/node_modules/create-remix/dist...
created build/node_modules/create-remix/dist in 249ms

packages/remix/index.ts β†’ build/node_modules/remix/dist...
created build/node_modules/remix/dist in 17ms

packages/remix/index.ts β†’ build/node_modules/remix/dist/esm...
created build/node_modules/remix/dist/esm in 13ms

...

packages/remix-server-runtime/index.ts β†’ build/node_modules/@remix-run/server-runtime/dist...
created build/node_modules/@remix-run/server-runtime/dist in 268ms

packages/remix-server-runtime/index.ts β†’ build/node_modules/@remix-run/server-runtime/dist/esm...
created build/node_modules/@remix-run/server-runtime/dist/esm in 236ms

packages/remix-vercel/index.ts β†’ build/node_modules/@remix-run/vercel/dist...
created build/node_modules/@remix-run/vercel/dist in 16ms
🚚  Copying remix deps...
🎬  Running Remix Init...
? Run your Remix site with: (Use arrow keys)
❯ Netlify Functions - Choose this for stable support for production sites 
  Netlify Edge Functions (beta) - Try this for improved performance on non-critical sites

Test Remix Running on Netlify Functions

  1. From the prompt, select Netlify Functions and press the ENTER Key
🎬  Running Remix Init...
? Run your Remix site with: (Use arrow keys)
❯ Netlify Functions - Choose this for stable support for production sites 
  Netlify Edge Functions (beta) - Try this for improved performance on non-critical sites
  1. The project gets installed into a folder called playground-*, e.g.
🚚  Copying remix deps...
🎬  Running Remix Init...
? Run your Remix site with: Netlify Functions - Choose this for stable support for production sites
βœ…  Done! Now in one terminal run `yarn watch` in the root of the remix repo and in another cd into ./playground/playground-1669172891010 and run `npm run dev` and you should be set.
✨  Done in 279.53s.
  1. Change to the directory of the new project for Remix and Netlify Functions, e.g. ./playground/playground-1669172891010
  2. DO NOT RUN NPM INSTALL (the yarn watch that is currently running is handling the dependencies for node_modules
  3. Run chmod 777 node_modules/.bin/remix (for some reason in the playground you need to do this, at least on my machine)
  4. Run netlify build --offline to build the project.
➜ netlify build                        

────────────────────────────────────────────────────────────────
  Netlify Build                                                 
────────────────────────────────────────────────────────────────

❯ Version
  @netlify/build 28.3.1

❯ Flags
  dry: false
  offline: false

❯ Current directory
  /Users/nicktaylor/dev/work/remix/playground/playground-1669172891010

❯ Config file
  /Users/nicktaylor/dev/work/remix/playground/playground-1669172891010/netlify.toml

❯ Context
  production

────────────────────────────────────────────────────────────────
  1. build.command from netlify.toml                            
────────────────────────────────────────────────────────────────

$ remix build
Building Remix app in production mode...
Built in 204ms

(build.command completed in 1.2s)

────────────────────────────────────────────────────────────────
  2. Functions bundling                                         
────────────────────────────────────────────────────────────────

The Netlify Functions setting targets a non-existing directory: netlify/functions

Packaging Functions from .netlify/functions-internal directory:
 - server.js


(Functions bundling completed in 940ms)

────────────────────────────────────────────────────────────────
  Netlify Build Complete                                        
────────────────────────────────────────────────────────────────

(Netlify Build completed in 2.1s)
  1. Run netlify dev --framework=#static
➜ netlify dev --framework=#static
β—ˆ Netlify Dev β—ˆ
β—ˆ Ignored general context env var: LANG (defined in process)
β—ˆ Using simple static server because '[dev.framework]' was set to '#static'
β—ˆ Running static server from "playground-1669336525432/public"
β—ˆ Loaded function server http://localhost:8888/.netlify/functions/server.
β—ˆ Functions server is listening on 61189
β—ˆ Setting up local development server

────────────────────────────────────────────────────────────────
  Netlify Build                                                 
────────────────────────────────────────────────────────────────

❯ Version
  @netlify/build 28.3.1

❯ Flags
  {}

❯ Current directory
  /Users/nicktaylor/dev/work/remix/playground/playground-1669336525432

❯ Config file
  /Users/nicktaylor/dev/work/remix/playground/playground-1669336525432/netlify.toml

❯ Context
  dev

────────────────────────────────────────────────────────────────
  1. Run command for local development                          
────────────────────────────────────────────────────────────────


β—ˆ Static server listening to 3999

(dev.command completed in 1ms)

Adding local .netlify folder to .gitignore file...

   β”Œβ”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”
   β”‚                                                 β”‚
   β”‚   β—ˆ Server now ready on http://localhost:8888   β”‚
   β”‚                                                 β”‚
   β””β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”˜

β—ˆ Rewrote URL to /.netlify/functions/server
Request from ::1: GET /.netlify/functions/server
Response with status 200 in 77 ms.
β—ˆ Rewrote URL to /.netlify/functions/server
Request from ::1: GET /.netlify/functions/server
Response with status 404 in 8 ms.
  1. The project opens at http://localhost:8888

CleanShot 2022-11-22 at 22 20 01

Test Remix Running on Netlify Edge Functions

  1. From the prompt, select Netlify Edge Functions and press the ENTER Key
🎬  Running Remix Init...
? Run your Remix site with: (Use arrow keys)
  Netlify Functions - Choose this for stable support for production sites 
❯ Netlify Edge Functions (beta) - Try this for improved performance on non-critical sites
  1. The project gets installed into a folder called playground-*, e.g.
🚚  Copying remix deps...
🎬  Running Remix Init...
? Run your Remix site with: Netlify Edge Functions (beta) - Try this for improved performance on non-critical sites
βœ…  Done! Now in one terminal run `yarn watch` in the root of the remix repo and in another cd into ./playground/playground-1669173817942 and run `npm run dev` and you should be set.
✨  Done in 46.54s.
  1. Change to the directory of the new project for Remix and Netlify Functions, e.g. ./playground/playground-1669172891010
  2. DO NOT RUN NPM INSTALL (the yarn watch that is currently running is handling the dependencies for node_modules
  3. Run chmod 777 node_modules/.bin/remix (for some reason in the playground you need to do this, at least on my machine)
  4. Run netlify build --offline to build the project.
  5. Run netlify dev --framework=#static
  6. The project opens at http://localhost:8888

CleanShot 2022-11-22 at 22 20 01

nickytonline avatar May 05 '22 22:05 nickytonline

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 CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at [email protected].

Thanks!

- The Remix team

remix-cla-bot[bot] avatar May 05 '22 22:05 remix-cla-bot[bot]

Thank you for signing the Contributor License Agreement. Let's get this merged! πŸ₯³

remix-cla-bot[bot] avatar May 05 '22 22:05 remix-cla-bot[bot]

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 dev branch.

nickytonline avatar May 06 '22 00:05 nickytonline

@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. For inspiration, you can have a look at the other adapters.

MichaelDeBoey avatar May 06 '22 00:05 MichaelDeBoey

~@mcansh, I'm currently adding the updated Edge template files, and I noticed that it looks like our template is the only one currently using hydrateRoot aside from the playground example that @kentcdodds added in #2663. Should I stick to just using hydrate for now like the other templates? We're also the only ones currently using renderToReadableStream in entry.server.tsx.~ nvm, I'm going to keep it since React 18 works with Remix and the template.

nickytonline avatar May 06 '22 15:05 nickytonline

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 package.json:

{
"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.

pcattori avatar May 06 '22 16:05 pcattori

I'm still testing things out locally.

nickytonline avatar May 06 '22 20:05 nickytonline

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.

From Discord:

  1. update packages/remix-dev/cli/create.ts to the following
-  ["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;
+        }
+      }
+    }
+  });
  1. Run REMIX_USE_BUILD_DEPS=1 node ./build/node_modules/create-remix/cli.js $HOME/Desktop/test --template ./templates/netlify

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.

❯ npx create-remix@latest --template ./remix/templates/netlify
Debugger attached.
Debugger attached.
? Where would you like to create your app? ./my-remix-app
? Do you want me to run `npm install`? Yes
β ‹ Creating your app…Debugger attached.
Waiting for the debugger to disconnect...
Debugger attached.
npm ERR! code ETARGET
npm ERR! notarget No matching version found for @remix-run/netlify-edge@^1.4.3.
npm ERR! notarget In most cases you or one of your dependencies are requesting
npm ERR! notarget a package version that doesn't exist.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/nicktaylor/.npm/_logs/2022-05-06T20_34_25_062Z-debug-0.log
Waiting for the debugger to disconnect...
Command failed: npm install
Waiting for the debugger to disconnect...
Waiting for the debugger to disconnect...

nickytonline avatar May 06 '22 20:05 nickytonline

Just as a heads up, @jacob-ebey found a way to get Netlify Edge working purely through the template and without modifying the compiler.

So we should stop efforts to modify the Remix compiler for Netlify Edge support and focus on doing so via templates instead.

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 edge-functions.

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 avatar May 10 '22 13:05 nickytonline

@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.

MichaelDeBoey avatar May 17 '22 10:05 MichaelDeBoey

@nickytonline #3117 is now merged, so I think it would be a good idea to rebase this PR onto latest dev & resolve conflicts.

We can see what needs to be done from that point on.

MichaelDeBoey avatar May 17 '22 18:05 MichaelDeBoey

@nickytonline https://github.com/remix-run/remix/pull/3117 is now merged, so I think it would be a good idea to rebase this PR onto latest dev & resolve conflicts.

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.

nickytonline avatar May 17 '22 18:05 nickytonline

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 avatar May 18 '22 11:05 nickytonline

@nickytonline If you need any help, I'm happy to help out.

MichaelDeBoey avatar May 18 '22 19:05 MichaelDeBoey

@MichaelDeBoey @pcattori, for testing this, are @mcansh's guidelines in https://github.com/remix-run/remix/pull/3104#issuecomment-1120003964 still the best way to test all this locally?

nickytonline avatar May 20 '22 01:05 nickytonline

⚠️ No Changeset found

Latest commit: f96d44fe9ad0278181e76c0c6edf84dacac8b1a4

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

changeset-bot[bot] avatar Jul 14 '22 22:07 changeset-bot[bot]

These are the two main issues I'm looking at right now @ascorbic. We can chat about them during our sync tomorrow. To replicate these issues, just follow the steps in the testing strategy in the PR description.

  • ~~For Netlify functions, I need to see why the Netlify dev server shuts down immediately after serving the page, i.e.β—ˆ "npm run dev" exited with code 0. Shutting down Netlify Dev server**~~ FIXED

  • ~~For Edge functions I need to see why the import for createRequestHandler in server.js is always undefined even though the code is all there.~~ FIXED

Could not load edge function at '/Users/nicktaylor/dev/work/remix/playground/playground-1669173817942/.netlify/edge-functions/server.js'
TypeError: (void 0) is not a function
    at file:///Users/nicktaylor/dev/work/remix/playground/playground-1669173817942/.netlify/edge-functions/server.js:5450:30

The TypeError: (void 0) is not a function is because in server.js createRequestHandler is undefined when imported, i.e.

import * as build from "@remix-run/dev/server-build";
import { createRequestHandler } from "@remix-run/netlify-edge";

export default createRequestHandler({
  build,
  // process.env.NODE_ENV is provided by Remix at compile time
  mode: process.env.NODE_ENV,
});

It's odd because in yarn watch in the other terminal, you can see these dependencies being built. Maybe it's something to do with running the Netlify template in the playground as I believe @chaance mentioned in Discord it's not really set up for this, it's typically someone trying a template that is not one of the templates that ship with Remix, e.f. Netlify, Cloudflare etc.

Full build log:

➜ ntl build                        

────────────────────────────────────────────────────────────────
  Netlify Build                                                 
────────────────────────────────────────────────────────────────

❯ Version
  @netlify/build 28.3.1

❯ Flags
  dry: false
  offline: false

❯ Current directory
  /Users/nicktaylor/dev/work/remix/playground/playground-1669173817942

❯ Config file
  /Users/nicktaylor/dev/work/remix/playground/playground-1669173817942/netlify.toml

❯ Context
  production

────────────────────────────────────────────────────────────────
  1. build.command from netlify.toml                            
────────────────────────────────────────────────────────────────

$ remix build
Building Remix app in production mode...
The path "@remix-run/netlify-edge" is imported in server.js but "@remix-run/netlify-edge" was not found in your node_modules. Did you forget to install it?
Built in 230ms

(build.command completed in 1.2s)

────────────────────────────────────────────────────────────────
  2. Functions bundling                                         

remix/playground/playground-1669173817942 on ξ‚  nickytonline/chore-update-netlify-remix-template [!?] via  v16.17.1 
➜ ntl build

────────────────────────────────────────────────────────────────
  Netlify Build                                                 
────────────────────────────────────────────────────────────────

❯ Version
  @netlify/build 28.3.1

❯ Flags
  dry: false
  offline: false

❯ Current directory
  /Users/nicktaylor/dev/work/remix/playground/playground-1669173817942

❯ Config file
  /Users/nicktaylor/dev/work/remix/playground/playground-1669173817942/netlify.toml

❯ Context
  production

────────────────────────────────────────────────────────────────
  1. build.command from netlify.toml                            
────────────────────────────────────────────────────────────────

$ remix build
Building Remix app in production mode...
Built in 195ms

(build.command completed in 904ms)

────────────────────────────────────────────────────────────────
  2. Functions bundling                                         
────────────────────────────────────────────────────────────────

The Netlify Functions setting targets a non-existing directory: netlify/functions

(Functions bundling completed in 1ms)

────────────────────────────────────────────────────────────────
  3. Edge Functions bundling                                    
────────────────────────────────────────────────────────────────

Packaging Edge Functions from .netlify/edge-functions directory:
 - server
Could not load edge function at '/Users/nicktaylor/dev/work/remix/playground/playground-1669173817942/.netlify/edge-functions/server.js'
TypeError: (void 0) is not a function
    at file:///Users/nicktaylor/dev/work/remix/playground/playground-1669173817942/.netlify/edge-functions/server.js:5450:30

(Edge Functions bundling completed in 410ms)

────────────────────────────────────────────────────────────────
  Netlify Build Complete                                        
────────────────────────────────────────────────────────────────

(Netlify Build completed in 1.3s)

nickytonline avatar Nov 23 '22 03:11 nickytonline

I'm just looking into what tests I need to write, but Remix running on Netlify Edge functions is working if you follow the steps in the testing section of the PR description.

Also, for the currently failing tests, there are some errors related to Deno style imports. Is this a known issue on the dev branch?

Error: packages/remix-deno/index.ts(2,42): error TS2691: An import path cannot end with a '.ts' extension. Consider importing './sessions/fileStorage.js' instead.
Error: packages/remix-deno/index.ts(7,8): error TS2691: An import path cannot end with a '.ts' extension. Consider importing './server.js' instead.
Error: packages/remix-deno/index.ts(14,8): error TS2691: An import path cannot end with a '.ts' extension. Consider importing './implementations.js' instead.

nickytonline avatar Nov 25 '22 00:11 nickytonline

For testing, I'm looking at the deployment target, i.e. https://github.com/nickytonline/remix/blob/c9afffb830d48be688de9d7d0dceb503f128b5bc/.github/workflows/deployments.yml and have added a job for netlify-edge, but one thing I'm wondering is there's no way for me to choose Netlify Functions vs. Netlify Edge Functions in our template because as far as I know, the inquirer package does not allow you to feed in a set of answers.

🎬  Running Remix Init...
? Run your Remix site with: (Use arrow keys)
  Netlify Functions - Choose this for stable support for production sites 
❯ Netlify Edge Functions (beta) - Try this for improved performance on non-critical sites

I think what will probably make more sense is to have the two templates separate instead of modifying the Netlify Functions one for Netlify Edge's needs. So when the Remix CLI runs, you'd see something like this:

➜ npx create-remix 
? Where would you like to create your app? ./my-remix-app
? What type of app do you want to create? Just the basics
? Where do you want to deploy? Choose Remix App Server if you're unsure; it's easy to change deployment targets. 
  Express Server 
  Architect (AWS Lambda) 
  Fly.io 
  Netlify 
❯ Netlify Edge Functions
  Vercel 
  Cloudflare Pages 
  Cloudflare Workers 
(Move up and down to reveal more choices)

Thoughts or maybe I'm missing something where I can still keep the one template and somehow toggle which one is deployed?

nickytonline avatar Nov 25 '22 17:11 nickytonline

Any thoughts on this @MichaelDeBoey since you're around? https://github.com/remix-run/remix/pull/3104#issuecomment-1327745439

nickytonline avatar Nov 25 '22 18:11 nickytonline

@nickytonline We have an interactWithShell helper in our CLI tests that can probably help you here https://github.com/remix-run/remix/blob/8f7100b2d5c40d5b2f0856958898cab70ef9d407/packages/remix-dev/tests/cli-test.ts#L282-L362

Usage examples:

  • https://github.com/remix-run/remix/blob/8f7100b2d5c40d5b2f0856958898cab70ef9d407/packages/remix-dev/tests/cli-test.ts#L217-L224
  • https://github.com/remix-run/remix/blob/8f7100b2d5c40d5b2f0856958898cab70ef9d407/packages/remix-dev/tests/cli-test.ts#L242-L248

MichaelDeBoey avatar Nov 25 '22 19:11 MichaelDeBoey

I'm still seeing these errors in CI checks even though it's code I haven't touched. Is this a known issue with CI when a PR is based off the dev branch?

Also, for the currently failing tests, there are some errors related to Deno style imports. Is this a known issue on the dev branch?

Error: packages/remix-deno/index.ts(2,42): error TS2691: An import path cannot end with a '.ts' extension. Consider importing './sessions/fileStorage.js' instead.
Error: packages/remix-deno/index.ts(7,8): error TS2691: An import path cannot end with a '.ts' extension. Consider importing './server.js' instead.
Error: packages/remix-deno/index.ts(14,8): error TS2691: An import path cannot end with a '.ts' extension. Consider importing './implementations.js' instead.

e.g. https://github.com/remix-run/remix/actions/runs/3568498913/jobs/5997434046#step:6:87

nickytonline avatar Nov 28 '22 22:11 nickytonline

I'm still seeing these errors in CI checks even though it's code I haven't touched. Is this a known issue with CI when a PR is based off the dev branch?

@nickytonline These failures shouldn't happen normally

MichaelDeBoey avatar Nov 28 '22 22:11 MichaelDeBoey

I'm still seeing these errors in CI checks even though it's code I haven't touched. Is this a known issue with CI when a PR is based off the dev branch?

@nickytonline These failures shouldn't happen normally

Hmm, not sure what's up then. I'll dig into this once the CI stuff is sorted.

nickytonline avatar Nov 28 '22 22:11 nickytonline

I had a quick look at the test failures and they are caused by this PR. You can repro them in isolation by running tsc -b packages/remix-netlify-edge. The problem is that the Netlify Edge package imports from the Deno package, which uses Deno-style imports. This means that tsc can't typecheck it. It also surprises me that it compiles properly. I guess rollup is more forgiving?

ascorbic avatar Nov 30 '22 08:11 ascorbic

Unfortunately it looks like https://github.com/microsoft/TypeScript/issues/37582 isn't going to be a viable answer. It didn't make it into TypeScript 4.9, so we won't see it til March.

ascorbic avatar Nov 30 '22 08:11 ascorbic

The "fix" is to remove netlify-edge from the root tsconfig, so that it's not included in the typechecking step. That's all it's used for. The Deno package isn't currently typechecked either. Adding a deno check task for both packages could be a good addition, but I think that would be a separate PR, because most of the work would be on the deno package rather than this one.

ascorbic avatar Nov 30 '22 12:11 ascorbic

The "fix" is to remove netlify-edge from the root tsconfig, so that it's not included in the typechecking step. That's all it's used for. The Deno package isn't currently typechecked either. Adding a deno check task for both packages could be a good addition, but I think that would be a separate PR, because most of the work would be on the deno package rather than this one.

Thanks @ascorbic! I've gone ahead and made that change so CI can start getting some βœ… across the board. I think this make sense for the moment @MichaelDeBoey @jacob-ebey @pcattori given @ascorbic's note about TS

Unfortunately it looks like https://github.com/microsoft/TypeScript/issues/37582 isn't going to be a viable answer. It didn't make it into TypeScript 4.9, so we won't see it til March.

but definitely open to other suggestions if you have any.

nickytonline avatar Nov 30 '22 13:11 nickytonline

I went ahead and added a separate PR to add typechecking for Deno. https://github.com/remix-run/remix/pull/4715

ascorbic avatar Nov 30 '22 13:11 ascorbic

I think that test failure is unrelated, and is caused by a race condition in the main build script. The globals.d.ts file is created and copied inside a single Promise.all, and on mac OS it's trying to copy it before it's created

ascorbic avatar Nov 30 '22 13:11 ascorbic