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

Adapt Superflare to work with Wrangler 3 and Remix v2 (vite compiler) #66

Merged
merged 157 commits into from
Nov 15, 2024

Conversation

acusti
Copy link
Contributor

@acusti acusti commented Jul 19, 2024

This PR:

  • updates the superflare/remix package + remix template + docs site to work with remix v2 and the vite compiler (based on the official cloudflare workers template)
  • updates all packages to the latest v5.x version of typescript and fixes some new typescript errors
  • updates the superflare CLI to work with wrangler v3 commands and miniflare v3 APIs. this entailed changes to some of the CLI command APIs:
    • the migrate command’s --db option is now the name of the DB binding, not a path to the actual local DB file
    • the dev command now runs remix vite:dev for the main server + wrangler dev in parallel to enable Durable Objects
  • removes direct dependency on better-sqlite3 in favor of wrangler’s getPlatformProxy API
  • removes ./d1js/* vendored files
  • adds a new export to the @superflare/remix plugin, available at @superflare/remix/dev, which provides superflareDevProxyVitePlugin, a drop-in replacement for remix’s cloudflareDevProxyVitePlugin that supports superflare and its automatic session handling and auth features
  • upgrades to latest pnpm
  • upgrades to latest turbo
  • adopts new Static Assets for Workers feature

implements #62

@@ -88,14 +88,17 @@ export async function getManifest(
return JSON.parse(manifest);
}

function getNodeText(node: RenderableTreeNode) {
function getNodeText(renderableNode: RenderableTreeNode) {
if (typeof renderableNode === "string") return renderableNode;
Copy link
Contributor Author

@acusti acusti Jul 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jplhomer heads up that this line is a change in behavior. in the previous version of this function, if typeof node === "string", the function returned an empty string, which i thought might be a bug, but if it is in fact desired, i will just lump this check in with the conditions in the next if statement (on line 93) so that it also returns an empty string.

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
@acusti
Copy link
Contributor Author

acusti commented Nov 1, 2024

@jplhomer i just pushed up a change to the superflare dev command so that it doesn’t use npx (which was unnecessary/problematic in my testing) and suppresses the output from wrangler dev so that it looks like the user is just running remix vite:dev, which is much less confusing, so that first issue I mentioned 2 days ago looks to be resolved. i will have better assurance that it works outside of the superflare repo once there’s a new installable release from the publish-snapshot action on this PR for me to test.

this leaves the last known TODO item for me as the documentation, which i am happy to get started on once i have a better idea of what you think the plan should be for this PR.

v2.12.1 includes remix-run/remix#9976, which changed the API contract of the fromNodeRequest util that we will depend on in superflareDevProxyVitePlugin
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
fixes UTF 8 text encoding / rendering, at least when running the docs app locally
@acusti
Copy link
Contributor Author

acusti commented Nov 12, 2024

@jplhomer tl;dr could you please trigger the publish-snapshot workflow with the latest so that i can test out the latest fixes in my own project?


the gory details: i refactored the new superflareDevProxyVitePlugin to take over the core part of remix’s cloudflareDevProxyVitePlugin, which is the actual vite dev server middleware that is used to handle all requests. originally, i chose not to do so:

an alternative solution that would enable us to avoid requiring custom code in the developer’s entry.server.ts file would be to re-implement the cloudflareDevProxyVitePlugin as a part of the superflareDevProxyVitePlugin, so that we can make it call next() and then have a final registered middleware that commits the cookie and doesn’t call next() to serve as the server terminus. this was actually my original plan until i realized it would require duplicating all of this file into @superflare/remix, which seemed less than ideal, so i didn’t go with that. but it’s totally doable if that seems preferable to you.

but a couple of days ago, i noticed that the default logout route wasn’t working because it calls auth.logout() and returns a redirect, meaning that the entry.server.tsx handlers never get invoked and so the session never gets committed and the post-logout session data doesn’t get written to the cookie. this made it obvious that taking over the request handling portion of the dev proxy vite plugin was really the best and only way to handle this properly, plus i realized that the node-adapter.ts file that i didn’t want to have to duplicate inside superflare is actually published as @remix-run/dev/dist/vite/node-adapter.js, so the surface area of the code that has to be handled by superflare to make the dev server work correctly is actually pretty reasonable, and most of it is code to instantiate superflare.config, set up the load context, and then commit the session if isDirty() right before generating and streaming the response: https://github.com/jplhomer/superflare/pull/66/files#diff-d9a86b278811d9a84bf4a62d69017ae9f6000d2b9073e8100b7d0ee5e1562041R68-R103

other recent changes are adopting the new workers Static Assets feature, upgrading from turbo v1 → v2 (latest) and upgrading to the latest version of pnpm.

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)
@jplhomer
Copy link
Owner

jplhomer commented Nov 14, 2024

@acusti Just ran again! Thanks for pushing on this - things are crazy for me right now but I anticipate being able to take a look at this and getting it merged ASAP and switch the project into prerelease mode before cutting another major version.

also, allow it to be overwritten in case someone wants to make superflare work with a wrangler.toml
@acusti
Copy link
Contributor Author

acusti commented Nov 14, 2024

@jplhomer that’s great news, and thanks for triggering a new release! i will start in on updating the documentation. also, after using the new releases in my own project, i discovered that i wasn’t providing experimentalJsonConfig: true by default to wrangler’s getPlatformProxy, so i pushed up the fix for that issue. but aside from that, the new superflareDevProxyVitePlugin implementation is working great for me in dev.

Copy link
Owner

@jplhomer jplhomer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just phenomenal work. Can't believe you kept up with this. Thank you!

Comment on lines +68 to +70
// Import singleton-dependent modules via viteDevServer to get
// the same instance of the Config singleton class as in app code.
const superflare = await server.ssrLoadModule("superflare");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, super clever way to share between module graphs.

@jplhomer jplhomer merged commit 1b2ee7f into jplhomer:main Nov 15, 2024
3 checks passed
@acusti
Copy link
Contributor Author

acusti commented Nov 15, 2024

@jplhomer thanks so much for the kind words! but oh dear, it seems i broke the docs site; let me know if you want any help debugging it. i did have to change a number of things to get it to work with the upgraded stack.

shall i create a new branch to start on documentation updates? there are some subtle changes to document that should be trivial for new superflare consumers but should probably be called out for existing folks looking to upgrade in an upgrade guide or the like. things like with the latest wrangler, you have to add --remote when running your migrations or else they just get run locally (which i believe wasn’t a thing initially because D1 didn’t even support having a local instance, but i’m not sure). or that if you use build: { minify: true } in your vite config, you’ll cause your model names to get mangled and break everything.

@jplhomer
Copy link
Owner

@acusti no worries! The only errors I can get from the server are:

Screenshot 2024-11-15 at 2 22 29 PM

Which seems to correspond to a Cloudflare 1003, something about direct IP references which doesn't add up 🤔

It also fails locally when I pnpm start in the docs site: it's just a Remix 404 page. Very odd - works fine during local development.

I gave it about 30 minutes but can't seem to find any leads - since it's fresher in your mind, maybe you can uncover what might be happening differently in dev vs prod/local build?

@jplhomer
Copy link
Owner

Also yes, updating the docs would be great. We haven't added a changeset yet, so there's not a major hurry to get them up.

@jplhomer
Copy link
Owner

Update: Found the issue! For some reason, deploying via Cloudflare integration wasn't setting NODE_ENV=production like it used to in the GitHub action. Prepending the CF deploy command fixed things 👍

@acusti
Copy link
Contributor Author

acusti commented Nov 15, 2024

that’s odd, i haven’t needed to set that manually with other cloudflare workers remix projects. i figured the most likely thing to break with the docs site was the search integration, but that actually seems to work fine! except for a CSS thing that seems to be the result of the site not having the necessary styles for the DocSearch-VisuallyHiddenForAccessibility classname.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants