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

FOUC when using custom servers #7878

Closed
1 task done
marbemac opened this issue Nov 2, 2023 · 16 comments
Closed
1 task done

FOUC when using custom servers #7878

marbemac opened this issue Nov 2, 2023 · 16 comments

Comments

@marbemac
Copy link
Contributor

marbemac commented Nov 2, 2023

What version of Remix are you using?

2.2.0

Are all your remix dependencies & dev-dependencies using the same version?

  • Yes

Steps to Reproduce

  1. npx create-remix@latest --template remix-run/remix/templates/unstable-vite-express
  2. Create and import a css file that does something noticeable like set background color to red or something
  3. Load app, note flash on load, because CSS is not included in the server rendering and is only added on the client

Expected Behavior

No flash of unstyled content when using custom servers.

Actual Behavior

There is a flash of unstyled content when using custom servers.


This might be tricky to solve - the long standing approach to SSR w vite (the approach described in Vite's docs) is IMHO pretty cumbersome, would be amazing if remix could somehow smooth over the custom server + vite experience.

But that's a bit off topic, regarding this specific problem - one option is to somehow expose getStylesForUrl to the end user so that they can pass in the critical styles to their request handler similar to how the remix vite plugin does on request. The problem is that getStylesForUrl requires cssModulesManifest, which is scoped to the remixVitePlugin plugin.

The larger issue is that longer term, with the current custom server approach, any logic/features that remix's vite plugin covers in the configureServer middleware will need to be replicated by the end user in their custom server.

I'd be willing to try and tackle a solution here if ya'll have any preferred direction or other ideas!

@marbemac
Copy link
Contributor Author

marbemac commented Nov 2, 2023

Ok another option that I just tried out that works - splitting the configureServer middleware into two parts.

Part one includes all the non request handling logic, and is always run.

Part two actually handles the request, and is only run if user is not providing their own server.

Critical bits that the end user needs for their own custom server can be exposed on the req (or could expose a type safe WeakMap end user can use, that is keyed to requests).

Something along these lines:

type ViteReqCtx = {
  build: ServerBuild;
  criticalCss: string | undefined;
}

const viteReqCtx = new WeakMap<IncomingMessage, ViteReqCtx>();

// For end user to use in their app
export const getViteReqCtx = (req: IncomingMessage) => ViteReqCtx;

const remixVitePlugin = {
  // ...
  configureServer(vite) {
    vite.httpServer?.on("listening", () => {
      setTimeout(showUnstableWarning, 50);
    });

    return () => {
      vite.middlewares.use(async (req, res, next) => {
        try {
          // Invalidate all virtual modules
          vmods.forEach((vmod) => {
            let mod = vite.moduleGraph.getModuleById(
              VirtualModule.resolve(vmod)
            );

            if (mod) {
              vite.moduleGraph.invalidateModule(mod);
            }
          });

          let { url } = req;
          let [pluginConfig, build] = await Promise.all([
            resolvePluginConfig(),
            vite.ssrLoadModule(serverEntryId) as Promise<ServerBuild>,
          ]);

          let criticalCss = await getStylesForUrl(
            vite,
            pluginConfig,
            cssModulesManifest,
            build,
            url
          );

          viteReqCtx.set(req, {
            build,
            criticalCss,
          });

          next();
        } catch (error) {
          next(error);
        }
      });

      // Let user servers handle SSR requests in middleware mode
      if (vite.config.server.middlewareMode) return;

      vite.middlewares.use(async (req, res, next) => {
        let { build, criticalCss } = viteReqCtx.get(req)!;

        try {
          let handle = createRequestHandler(build, {
            mode: "development",
            criticalCss,
          });

          await handle(req, res);
        } catch (error) {
          next(error);
        }
      });
    };
  }
}

Then in the end user's custom server, they can get access to the critical css for the request, to pass into the remix request handlers. Although I noticed that at least the express adapter does not expose the criticalCss option (the node one does).

OR we could make this an adapter concern, pop these props on to the req, and the adapters could pass in the critical css if present.

Or to make it more runtime agnostic... could key the vite ctx by req url?

@marbemac
Copy link
Contributor Author

marbemac commented Nov 2, 2023

On second thought - might make more sense for the adapters to handle adding the critical css, since they already do that for prod? Could still accomplish this by popping the criticalCss info on the request context - the two step middleware described above is still relevant.

Otherwise it's just more dev specific logic that the end user has to manage in their custom server.

@pcattori pcattori added the vite label Nov 2, 2023
@gijsroge
Copy link

gijsroge commented Nov 4, 2023

@marbemac

https://remix.run/docs/en/main/future/vite#fix-up-css-imports

They explicitly said they solved this issue in the Remix plugin for Vite. However, I can reproduce the issue 🤔

@markdalgleish
Copy link
Member

@marbemac Thanks for raising this and looking into it so thoroughly! I've opened a PR addressing it: #7937

What's nice is that this doesn't require any code changes for consumers 🎉

@marbemac
Copy link
Contributor Author

marbemac commented Nov 9, 2023

@markdalgleish no problem!

Ok so I see you went with the two step middleware + popping critical css on the req (or, res.locals in your pr) ctx for use in adapters approach. This works for express apps - any reason we couldn't get it to work more generically and pop the info somewhere on the req or res (not just on res.locals which is an express specific thing)?

Reason I'm asking is because I'm actually using Hono. But this would be relevant to cloudflare, deno, etc - anything that operates on web standard req/res. Vite's a little awkward to use with these frameworks since it expects connect middleware, but it's not too difficult to plug connect middleware into things like Hono, which is what I'm doing during development. Can share a more concrete example if that would be helpful.

Thanks for getting things going w that PR!

@markdalgleish
Copy link
Member

@marbemac My PR was just a short term solution since it means our Vite Express template is fixed without introducing any new public APIs. In terms of a more general solution, if you can share a non-Express repo that would be super helpful to get me started on it quickly :)

@marbemac
Copy link
Contributor Author

Makes sense, it's a good first step :). I'm booked up today but will try and put together a basic playground repo to frame up the issue early next week.

Copy link
Contributor

🤖 Hello there,

We just published version 2.3.0-pre.0 which involves this issue. 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.3.0 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@markdalgleish markdalgleish reopened this Nov 16, 2023
@markdalgleish
Copy link
Member

Just re-opening this to track non-Express custom servers since the fix that went out was a more targeted short term fix for Express.

@danestves
Copy link

Currently facing that issue, I’m using Fastify as custom server 👀

@mcansh
Copy link
Collaborator

mcansh commented Nov 17, 2023

Currently facing that issue, I’m using Fastify as custom server 👀

if you’re using remix-fastify i have a prerelease out that allows for this, just requires a little bit of userland code which is linked to in the release notes https://github.com/mcansh/remix-fastify/releases/tag/%40mcansh%2Fremix-fastify%403.2.0-pre.0

@danestves
Copy link

Currently facing that issue, I’m using Fastify as custom server 👀

if you’re using remix-fastify i have a prerelease out that allows for this, just requires a little bit of userland code which is linked to in the release notes https://github.com/mcansh/remix-fastify/releases/tag/%40mcansh%2Fremix-fastify%403.2.0-pre.0

Tried that but getting this error 👀

@danestves
Copy link

@mcansh the problem was because I was using remix-flat-routes and it doesn't play well with vite

@pcattori pcattori added this to the Stabilize Vite milestone Nov 20, 2023
@pcattori pcattori removed this from the Stabilize Vite milestone Nov 21, 2023
@markdalgleish markdalgleish self-assigned this Nov 21, 2023
@markdalgleish
Copy link
Member

markdalgleish commented Nov 23, 2023

Fixed by #8076.

@marbemac @mcansh When this goes out in the next nightly, can you try this and report back? Your FOUC issue should now be fixed automatically without needing to manage the critical CSS at the adapter layer.

@marbemac
Copy link
Contributor Author

Awesome work @hi-ogawa and @markdalgleish! Looks great, love the approach. Only snag I ran into was to remember to explicitely set the mode arg to development if using createRequestHandler from @remix-run/server-runtime (rather than from @remix-run/express, which defaults it to NODE_ENV for you).

👌 💯

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

No branches or pull requests

6 participants