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

Proxy request.signal through in vite dev #9976

Merged
merged 4 commits into from
Sep 13, 2024

Conversation

brophdawg11
Copy link
Contributor

@brophdawg11 brophdawg11 commented Sep 11, 2024

Found this wasn't working right while testing #9975. Aligned with how we do it in the express adapter.

Closes #9438

Copy link

changeset-bot bot commented Sep 11, 2024

🦋 Changeset detected

Latest commit: 0689ce9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 16 packages
Name Type
@remix-run/dev Patch
create-remix Patch
remix Patch
@remix-run/architect Patch
@remix-run/cloudflare Patch
@remix-run/cloudflare-pages Patch
@remix-run/cloudflare-workers Patch
@remix-run/css-bundle Patch
@remix-run/deno Patch
@remix-run/eslint-config Patch
@remix-run/express Patch
@remix-run/node Patch
@remix-run/react Patch
@remix-run/serve Patch
@remix-run/server-runtime Patch
@remix-run/testing Patch

Not sure what this means? Click here to learn what changesets are.

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

@brophdawg11 brophdawg11 linked an issue Sep 12, 2024 that may be closed by this pull request
@brophdawg11 brophdawg11 merged commit a49b4c6 into dev Sep 13, 2024
9 checks passed
@brophdawg11 brophdawg11 deleted the brophdawg11/proxy-vite-signal branch September 13, 2024 14:54
@github-actions github-actions bot added the awaiting release This issue has been fixed and will be released soon label Sep 13, 2024
Copy link
Contributor

🤖 Hello there,

We just published version 2.12.1-pre.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Copy link
Contributor

🤖 Hello there,

We just published version 2.12.1 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@jvaill
Copy link

jvaill commented Sep 23, 2024

It's working better now! But I'm still having issues with the signal not being aborted sometimes.

Referencing this: sergiodxa/remix-utils#317 (comment)

@Eiley2
Copy link

Eiley2 commented Sep 25, 2024

Is there a workaround for this? Maybe check every once is a while if a connection is closed to abort that connection? I'm having the same issue with node and the updated version of remix in production too.

routes/sse.time.ts

import { LoaderFunctionArgs } from "@remix-run/node"
import { eventStream } from "remix-utils/sse/server"

export async function loader({ request }: LoaderFunctionArgs) {
  return eventStream(request.signal, function setup(send) {
    const timer = setInterval(() => {
      console.log(new Date().toLocaleTimeString())
      send({ event: "time", data: new Date().toLocaleTimeString() })
    }, 1000)

    return function clear() {
      console.log("SSE connection closed")
      clearInterval(timer)
    }
  })
}

and this is my route:

routes/time.tsx

import { useEventSource } from "remix-utils/sse/react"

export default Route(){
  let time = useEventSource("/sse/time", { event: "time" })
  if (!time) {
    time = new Date().toLocaleTimeString()
  }

return <p>{time}</p>

@jvaill
Copy link

jvaill commented Sep 25, 2024

It seems like the signal is working correctly until a new undici Request() is constructed. At that point, undici creates an underlying signal. That ends up getting passed to the remix handler.

Here's where Request() is created in remix.

Here's an issue I opened on the undici repo.

@jvaill
Copy link

jvaill commented Sep 26, 2024

Possible fix: #10030

@mkromann
Copy link

I am running into the same issue, and cannot get any EventSource to close with request.signal. Is there a workaround for how to handle this?

Right now it is a huge block for developing with SSE. We have to restart the whole app constantly.

acusti added a commit to acusti/superflare that referenced this pull request Nov 11, 2024
v2.12.1 includes remix-run/remix#9976, which changed the API contract of the fromNodeRequest util that we will depend on in superflareDevProxyVitePlugin
jplhomer added a commit to jplhomer/superflare that referenced this pull request Nov 15, 2024
…#66)

* ⬆️ Upgrade pnpm/wrangler/better-sqlite3

plus remove the @remix-run/dev patch

* Remove defunct NO_D1_WARNING env var

* ⬆️ Upgrade to latest remix v2.x

* ⬆️ Upgrade to latest typescript (v5.x)

* Remove defunct custom pnpm resolutions

* Update remix vite cloudflare-workers dependencies

• reference: https://github.com/remix-run/remix/tree/main/templates/cloudflare-workers
• also, update @superflare/remix peerDependencies
• remove unused @remix-run/serve dependency
• move @remix-run/server-runtime from devDependencies → dependencies
• upgrade to latest @cloudflare/workers-types + make it consistent across workspaces

* Upgrade to latest tailwind + markdoc

* Update docs site to work w/ remix v2 + vite (WIP)

* Update remix-cms example for remix v2 + vite (WIP)

* Update remix template for remix v2 + vite (WIP)

* Make generateTypesFromSqlite take D1Database db

…and refactor it to be an async function, plus add a D1DatabaseAdaptor for exposing a compatible API for a SqlliteDB instance

* Fix types via generics + conditional in Button.tsx

* Handle rename LoaderArgs → LoaderFunctionArgs

* Handle rename ActionArgs → ActionFunctionArgs

* Make meta functions remix v2 compatible

…plus take advantage of MetaFunction type’s generics

* Remove deprecated (now default) wrangler CLI flags

* 📝 Update docs site README env var info

* Regenerate docs site worker types with .dev.vars

* Fix types in docs site’s docs.server.ts

* Update docs site’s env var names in .env.example

* Adapt docs site’s loaders to new ctx shape

* Remove redundant wrangler dev script arg

From the wrangler docs:
“Only required if your wrangler.toml does not include a main key”
Source:
https://developers.cloudflare.com/workers/wrangler/commands/#dev

* Restore TS cloudflare worker’s entry file + name

reproduces the changes suggested in remix-run/remix#9774

* Remove defunct cloudflare Env type declarations

• the Env type is now generated by the wrangler types command and written to worker-configuration.d.ts
• note however that it doesn’t include CF_PAGES or APP_KEY, which are both used in examples/remix-cms/functions/[[remix]].ts

* Restore focus-visible polyfill

* 📝 Fix a typo in superflare/cli/new.ts

* Import + use types from @cloudflare/workers-types

* Adapt superflare’s loadContext to getPlatformProxy

* Add auth + session to AppLoadContext type

* Restore superflare handlers in remix template

* 📝 Update docs based on latest APIs

* Fix package.json types location for TS v.4.7+

* Add typecheck run script to packages/*

* Add missing @cloudflare/workers-types devDep

* ⬆️ Upgrade @cloudflare/workers-types

* Restore ambient workers types + original filenames

• no need to rename cloudlfare.env.d.ts → env.d.ts
• i kept the new env.d.ts name for the remix template in order to match the setup of the “official” cloudflare workers remix template (reference: https://github.com/remix-run/remix/tree/main/templates/cloudflare-workers)

* Disambiguate workers Request type (from global)

* Fix superflare build by making redis pkgs external

* ⬆️ Upgrade latest eslint-plugin-turbo

* Use createTestDatabase in /d1-types.test.ts

this means that superflare/cli/d1-database.ts’ createSQLiteDB util is no longer used anywhere

* Specify latest version for @miniflare/* deps

* Migrate from local /d1js → @miniflare/d1

* Add .dev.vars to .gitignore

* Remove global process type from examples/remix-cms

* Use createD1Database util in migrateHandler

* Use .dev.vars to define APP_KEY + regen types

* Simplify secure protocol check for session cookie

* Fix reference to remix build in remix-cms worker

* Replace non-existent Auth import → SuperflareAuth

* ⬆️ Upgrade to latest vite (v5.3.4)

* ⬆️ Upgrade latest docs site UI libs

the prism-react-renderer upgrade includes some breaking changes, so Fence.tsx and Hero.tsx have been updated to work with those changes as well

* Add ssr.noExternal to fix docs site’s build issues

based on algolia/docsearch#2259

* Restore remix-cms DO Channel export to main

* 🚿 Cleanup unneeded TS config values

* Restore queue + scheduled to remix-cms/worker.ts

* Fix naming of docs site’s root route’s index route

https://remix.run/docs/en/main/file-conventions/routes#basic-routes

* Filter out "_cf_KV" table when handling D1 models

* 🚿 Remove need for intermediate foo var

* Crane + file cabinet emojis need an extra space

🤷

* Replace better-sqlite3 → @miniflare/d1 in tests

this means no more direct dependency on better-sqlite3

* 🚿 Remove direct deps on better-sqlite3

* 🚿 Remove unused generate migration option

* Add getD1Database(dbName) using getPlatformProxy

* Refactor getD1Database from wrangler → miniflare@3

• the CLI migrate command doesn’t work if miniflare is imported statically
• adding it as a dependency ensures we get the correct types for miniflare v3.x (otherwise we get miniflare v2.x types)
• the previous version (using wrangler’s getPlatformProxy) was resulting in the following error:

service core:user:__WRANGLER_EXTERNAL_DURABLE_OBJECTS_WORKER: Worker "core:user:__WRANGLER_EXTERNAL_DURABLE_OBJECTS_WORKER"'s binding "Channel" refers to a service "core:user:worker", but no such service is defined.

* Refactor d1 commands from dbPath → dbName

• also refactor from using createD1Database → getD1Database
• refactor migrate --fresh option from using fs.rm() to delete the existing database file to dropping all user-facing tables tables from the existing database

* Explicitly exit CLI migrate command

* Update workers compatibility_date to latest

https://developers.cloudflare.com/workers/configuration/compatibility-dates/#properly-extract-blob-mime-type-from-content-type-headers

* Remove defunct <LiveReload> component

reference: https://remix.run/docs/en/main/guides/vite#hmr--hdr

* Remove inapplicable dev:superflare command

* Use vite’s built-in CSS handling in docs site

* Use CJS-compatible pluralize import

* Use remix vite:dev for superflare dev command

preserved the “Using (D1|R2) bindings: …” console logging and updated it to always print (not just in pages mode), because the remix vite:dev command doesn’t provide any of that info to the user

* Use superflare dev command for dev run script

* ⬆️ Upgrade to latest @clack/prompts

* Remove redundant ellipsis from spinner text

the spinner already animates a repeating ellipsis onto the end of the text as it spins, so having the text end with “...” is redundant

* Shorten spinner text to avoid text wrap bug

the bug: bombshell-dev/clack#132

* Remove defunct --legacy-peer-deps

all of the remix cloudflare packages have been upgraded to @cloudlfare/workers-types v4 (e.g. https://github.com/remix-run/remix/blob/main/packages/remix-cloudflare/package.json)

* Tighten up headers whitespace for readability

* Add snapshot publishing

* No compacting

* Allow `--repo` to be passed to `superflare new`

* feat: add support for account selection during `superflare new`

* Add getLoadContext to @superflare/remix

* Add superflareDevProxyVitePlugin for convenience

* Cast-to-any as type incompatibility workaround

* Use superflareDevProxyVitePlugin in vite.config

* Remove duplicate local load-context.ts

already supplied by @superflare/remix

* 🚿 Remove unused variable

* Add experimentalJsonConfig for wrangler.json

enables wrangler to pick up the bindings and provide them from getPlatformProxy

* Use getLoadContext to instantiate config singleton

getLoadContext is the common function invoked when handling requests across all environments (dev with vite and prod with workers)

* Use relative import to work pre-tsconfig

the superflare.config.ts is imported into the vite.config.ts, so the imports have to be resolved before the vite-tsconfig-paths plugin has been enabled, so `~/…` imports don’t work

* Resolve type error by updating config type

* Update types to resolve no implicit any errors

• HasMany’s save argument looks like Model[] | Model to me
• HasMany’s create argument is any, so i made that explicit in the map()

* Add script_name for Channel DO binding

wrangler’s getPlatformProxy requires the script_name in order to be able to proxy any Durable Object bindings: https://developers.cloudflare.com/workers/wrangler/api/#supported-bindings

* Clarify comments + use implicit return

* Differentiate index.types.ts type exports

* Replace type DefineConfigResult → DefineConfigReturn

allows us to only import the DefineConfigReturn type instead of importing the actual defineConfig function and using ReturnType<typeof defineConfig<Env>>

* Fix superflare-remix vite plugin + getLoadContext

• calling getLoadContext from a vite plugin means that it doesn’t execute in the same context as the actual application, so it gets its own Config class singleton
• update getLoadContext to take SuperflareAuth and SuperflareSession, then use viteDevServer.ssrLoadModule from the vite plugin to load both classes and provide them to getLoadContext
• use viteDevServer.ssrLoadModule from the vite plugin to load superflare.config.ts and provide it to getLoadContext

* Update vite.configs to not pass superflare.config

* Restore ~/ import in superflare.config.ts

the fixed implementation for initializing the config in getLoadContext means that the config file is now imported by vite and so the import alias now works

* Use entry.(client|server).tsx from remix example

sources:
https://github.com/remix-run/remix/blob/main/templates/cloudflare-workers/app/entry.client.tsx
https://github.com/remix-run/remix/blob/main/templates/cloudflare-workers/app/entry.server.tsx

* 📦 Update postcss + autoprefixer deps

* Fix tailwind styles in examples/remix-cms

* Fix input width on mobile + add a max width

• they were defaulting to .col-span-1 below 640px
• now they span across the full row at all breakpoints but with a 24rem (384px) max-width

* Add props.autoComplete to inputs + typeof action

* Fix getSessionCookie, consistent superflare import

adds getSessionCookie to AppLoadContext and provides it from load-context.ts

* Move config init to superflareDevProxyVitePlugin

also, bring over code to ensure session has a sessionId from superflare#handleFetch

* Ensure type-only superflare import + remove unused

* Add commitSession logic to entry.server.tsx

* Use wrangler’s getPlatformProxy in getD1Database

• reverts the changes implemented in 22f47d4
• the reason getPlatformProxy didn’t work when implemented in fe766b9 is that getPlatformProxy cannot handle Durable Objects if a script_name isn’t provided for the bindings (which was resolved in f5874c0)

* Refactor @superflare/remix types to take Env

• move @remix-run/cloudflare AppLoadContext definition to index.ts
• use AppLoadContext where necessary
• use WorkersRequest for accuracy + global Request for @remix-run/cloudflare’s createRequestHandler return value compatibility
• restore local ./load-context.ts to expand @remix-run/cloudflare’s AppLoadContext type to include a cloudflare property based on Env interface from ./worker-configuration.d.ts

* Remove defunct "superflare" import

* Fix import TextareaMarkdown for vite

related: vitejs/vite#2139 (comment)

* Fix types for routes/auth/hooks.ts’ useAdmin

* Upgrade isbot + eslint-(config|plugin)-turbo

* 🚿 Remove defunct concurrently deps

* 📦 Upgrade to latest concurrently

* Use experimental json config for wrangler deploy

* 🚿 Cleanup unused dependencies

* Export getLoadContext from @superflare/remix

* Move superflareDevProxyVitePlugin → remix-dev pkg

added new @superflare/remix-dev package to host the superflareDevProxyVitePlugin in such a way that it can be independently imported into users’ vite.config.ts file without then getting included in the built cloudflare workers bundle

* Use @superflare/remix-dev in remix-cms + template

the template dependency string will have to be changed from "workspace:*" → "*" once the new package has been published

* Update superflare dev to also run wrangler dev

fixes durable objects in dev: https://developers.cloudflare.com/workers/wrangler/api/#supported-bindings

* Run build serially (superflare/remix depends on superflare)

* Remove unused dependency from apps/site/

* Fix tsconfig’s cloudflare.env.d.ts include path

* Move vite dev plugin to @superflare/remix/dev

previously available as @superflare/remix-dev, but this is simpler

* Use @superflare/remix/dev in examples

* Use installed wrangler version (not latest) in CLI

* Use static assets for workers feature

* Remove defunct DBConfig type

* Fix d1_databases wrangler config field name

* Add script_name for Channel DO binding in CLI

same fix as f5874c0 but applied to the superflare CLI new command

wrangler’s getPlatformProxy requires the script_name in order to be able to proxy any Durable Object bindings: https://developers.cloudflare.com/workers/wrangler/api/#supported-bindings

* Adopt default remix v2 flat routing convention

https://remix.run/docs/en/main/file-conventions/routes

* Add generic types to template useActionData

* Drop migrations table for migrate --fresh option

* Don’t return handleQueue Promise.all

• the return value will be an array of undefined values of the same length as batch.messages, which doesn’t seem useful
* returning that array leads to a type error in the main worker file due to incompatibility with the return value of the ExportedHandlerQueueHandler<Env, unknown> type

* Fix request object type in handleFetch

* Fix  type of scheduled event argument

reference: https://github.com/cloudflare/workers-sdk/blob/main/packages/wrangler/templates/new-worker-scheduled.ts#L26

* DO script_name is worker app name (not file name)

follow-up to ebdbc99

* Fix script_name for DO binding in remix-cms example

• follow-up to f5874c0
• turns out the script_name should be the worker’s app name (not its file name)

* Separate @superflare/remix/dev completely from @superflare/remix

removing the ;./index' import from superflare-remix/dev.ts makes it so that there is no possible issue with using static `import {…} from 'superflare'` in superflare-remix/index.ts, because that file won’t be executed outside of the vite bundle/execution context

* Specify a wrangler version that supports static assets

* Fix superflare dev command + suppress wrangler dev io

* Add @remix-run/server-runtime, use latest pnpm

* Specify minimum remix version for devProxyVite plugin

v2.12.1 includes remix-run/remix#9976, which changed the API contract of the fromNodeRequest util that we will depend on in superflareDevProxyVitePlugin

* Fix committing session changes on response in dev

using entry.server.tsx meant that sessions weren’t committed for redirects, e.g. the default logout route action, which calls
```
auth.logout();
return redirect("/");
```
this commit takes over the middleware portion of the cloudflareDevProxyVitePlugin logic and frames the request/response lifecycle with superflare.config initialization + getLoadContext before the request handler is invoked, and commit session cookie handling after

* Ensure <meta charset> is first thing rendered

fixes UTF 8 text encoding / rendering, at least when running the docs app locally

* Include “.md” ext in default docs filepath

* Upgrade to latest version of turbo (2.x)

* 🚿 Strip down entry.server.tsx files

* Add lang to all code blocks for prism-react-renderer

not having a language identifier results in:

TypeError: Cannot read properties of undefined (reading 'toLowerCase')
    at Highlight (/Users/foo/superflare/node_modules/.pnpm/[email protected][email protected]/node_modules/prism-react-renderer/src/components/highlight.ts:14:30)

* Fix server-runtime imports (should be cloudflare)

* Add better-sqlite3 devDependency in packages/superflare

fixes tests locally on my machine

* Make getPlatformProxy experimentalJsonConfig: true

also, allow it to be overwritten in case someone wants to make superflare work with a wrangler.toml

* Add proper migrations to Remix CMS

---------

Co-authored-by: Josh Larson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

request.signal is never fired in dev server
5 participants