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

Fix serverBundles issue with multiple browser manifests #8864

Merged
merged 2 commits into from
Feb 23, 2024

Conversation

markdalgleish
Copy link
Member

@markdalgleish markdalgleish commented Feb 23, 2024

This issue meant that each server bundle generated a separate subset of the browser manifest rather than generating a single unified browser manifest. To fix this issue I've had to split the singular Remix manifest into a separate manifest for both client and server. This ensures that while the server bundle only contains a subset of the manifest, the client always receives the full manifest.

cc @TooTallNate.

Copy link

changeset-bot bot commented Feb 23, 2024

🦋 Changeset detected

Latest commit: 6c03682

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

@@ -26,7 +26,7 @@ import {
type RemixConfig as ResolvedRemixEsbuildConfig,
resolveConfig as resolveRemixEsbuildConfig,
} from "../config";
import { type Manifest as BrowserManifest } from "../manifest";
import { type Manifest as RemixManifest } from "../manifest";
Copy link
Member Author

Choose a reason for hiding this comment

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

This type has been renamed for clarity since the manifest type is shared between client and server, and especially now that its contents can differ too.

// For server bundle builds, override the relevant config. This lets us run
// multiple server builds with each one targeting a subset of routes.
if (serverBundleBuildConfig) {
routes = serverBundleBuildConfig.routes;
Copy link
Member Author

Choose a reason for hiding this comment

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

This was the crux of the issue. Both client and server received a subset of routes when in reality only the server should have a subset.

Comment on lines +697 to +702
let routes = ctx.serverBundleBuildConfig
? // For server bundle builds, the server build should only import the
// routes for this bundle rather than importing all routes
ctx.serverBundleBuildConfig.routes
: // Otherwise, all routes are imported as usual
ctx.remixConfig.routes;
Copy link
Member Author

Choose a reason for hiding this comment

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

This is one of two main parts of the fix. Rather than modifying the resolved routes for the Remix config — which incorrectly impacts the client, not just the server — we use a subset of routes in the server build file.

Comment on lines +851 to +857
// The server manifest is the same as the browser manifest, except for
// server bundle builds which only includes routes for the current bundle,
// otherwise the server and client have the same routes
let remixServerManifest: RemixManifest = {
...remixBrowserManifest,
routes: serverRoutes,
};
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the other main part of the fix. Here we only provide a subset of the manifest on the server when using server bundles, while still writing the full manifest to disk for the client.

@markdalgleish markdalgleish merged commit 0695c43 into dev Feb 23, 2024
9 checks passed
@markdalgleish markdalgleish deleted the markdalgleish/server-bundle-manifest branch February 23, 2024 11:50
@github-actions github-actions bot added the awaiting release This issue has been fixed and will be released soon label Feb 23, 2024
Copy link
Contributor

🤖 Hello there,

We just published version 2.8.0-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!

EndangeredMassa pushed a commit to vercel/vercel that referenced this pull request Feb 28, 2024
Adds support for Remix apps which use the new Remix Vite plugin.

* The vanilla Remix + Vite template deploys correctly out-of-the-box,
however only a single Node.js function will be used, and a warning will
be printed saying to configure the `vercelPreset()` Preset.
* When used in conjunction with the `vercelPreset()` Preset
(vercel/remix#81), allows for the application to
utilize Vercel-specific features, like per-route `export const config`
configuration, including multi-runtime (Node.js / Edge runtimes) within
the same app.

## To test this today

1. Generate a Remix + Vite project from the template:
    ```
    npx create-remix@latest --template remix-run/remix/templates/vite
    ```
1. Install `@vercel/remix`:
    ```
    npm i --save-dev @vercel/remix
    ```
1. **(Before Remix v2.8.0 is released)** - Update the `@remix-run/dev`
dependency to use the "pre" tag which contains [a bug
fix](remix-run/remix#8864):
    ```
    npm i --save--dev @remix-run/dev@pre @remix-run/serve@pre
    ```
1. Configure the `vercelPreset()` in the `vite.config.ts` file:
    ```diff
    --- a/vite.config.ts
    +++ b/vite.config.ts
    @@ -1,10 +1,11 @@
     import { vitePlugin as remix } from "@remix-run/dev";
     import { installGlobals } from "@remix-run/node";
     import { defineConfig } from "vite";
    +import { vercelPreset } from "@vercel/remix/vite";
     import tsconfigPaths from "vite-tsconfig-paths";
    
     installGlobals();
    
     export default defineConfig({
    -  plugins: [remix(), tsconfigPaths()],
    +  plugins: [remix({ presets: [vercelPreset()] }), tsconfigPaths()],
     });
    ```
1. Create a new Vercel Project in the dashboard, and ensure the Vercel
preset is set to "Remix" in the Project Settings. The autodetection will
work correctly once this PR is merged, but for now it gets incorrectly
detected as "Vite" preset.
* **Hint**: You can create a new empty Project by running the `vercel
link` command.
<img width="545" alt="Screenshot 2024-02-27 at 10 37 11"
src="https://github.com/vercel/vercel/assets/71256/f46baf57-5d97-4bde-9529-c9165632cb30">
1. Deploy to Vercel, setting the `VERCEL_CLI_VERSION` environment
variable to use the changes in this PR:
    ```
vercel deploy -b
VERCEL_CLI_VERSION=https://vercel-git-tootallnate-zero-1217-research-remix-v-next-vite.vercel.sh/tarballs/vercel.tgz
    ```
Copy link
Contributor

🤖 Hello there,

We just published version 2.8.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!

@github-actions github-actions bot removed the awaiting release This issue has been fixed and will be released soon label Feb 28, 2024
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.

2 participants