feat(remix-server-runtime): pass AppLoadContext to the server's handleRequest function

[axel-habermaier]

The AppLoadContext instance can optionally be set via the server adapter's getLoadContext() function. It is subsequently passed to all actions, loaders as well as the handleDataRequest function. It is, however, not passed to the handleRequest function.

This PR also passes the load context to the handleRequest function so that the context is available everywhere in a consistent manner.

• Docs
• Tests

[remix-cla-bot]

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

[kiliman]

Hmm.. not sure why it's not working for you. I setup a codesandbox that uses getLoadContext and I am able to get the value from loader({context})
https://codesandbox.io/s/remix-getloadcontext-f8s1s0?file=/app/routes/index.tsx

// server/index.js

const getLoadContext = req => ({
  test: `This is from getLoadContext: ${new Date(Date.now()).toISOString()}`,
})

function handleRequest(req, res, next) {
  let build = require('./build')
  if (MODE !== 'production') {
    purgeRequireCache()
  }
  return createRequestHandler({
    build,
    getLoadContext,
    mode: MODE,
  })(req, res, next)
}

[ghost]

@kiliman: Thanks for the example. But this is not what this PR does. The load context is already available to all loader, and action functions, as your example shows. It is not, however, available in the function that renders your React component tree on the server in entry.server.tsx. From the PR's diff:

This PR adds the load context as the last parameter of the default function in entry.server.tsx. That way, the change is API backward compatible (because in JavaScript, you can always omit unused parameters in functions that get called with more parameters).

[kiliman]

Hmm. Sorry, I guess I misunderstood what you were doing. Ok.

The handleDataRequest export API came later. I believe the person that implemented the export used the same API as the internal handler, therefore loadContext came along for the ride. I'm not sure that was the intention as the context is already being passed to the loader directly. It was added in this commit 2f4d8fd (server-runtime/server.js lines 158-163)

But the component is also rendered on the client. Your context contains information from your adapter to be used by your loaders and actions. If there's anything in context that is needed by the component, it should be returned by the loader/action.

[ghost]

The problem is that I can't rely on loaders to provide data - they're don't, for instance, in case of 404s or top-level errors. Nevertheless, we need some tenant-specific info in these cases (tenant name, logo, URL, language, color theme, and even some user-specific stuff, etc.). Since we can't pass this info via loaders in all cases, we have to partially re-implement Remix' encoding of loader data in the HTML, in our case encoding the tenant info in the topmost component into the HTML, wrapping <RemixServer/> in entry.server.tsx. In entry.client.tsx, we do the opposite and wrap <RemixClient/> in a component that reads the tenant info from the HTML and makes it available everywhere via a React context.

We would like to uniformly pass the tenant-specific data in entry.server.tsx via the load context, that is normally used by the loaders to pass the data to the frontend. Right now, we have to use an ugly workaround with global variables and a per-request look-up table for the correct context, requiring this context to be removed from the lookup when the requests ends to avoid memory leaks.

Obviously, if we can't even load the tenant info due to errors, we have to fall back to an unstyled, tenant-unspecific error. But that is extremely unlikely, as our tenant info is stored in-memory and there are not many possibilities for failures. 404s and other internal server errors, however, are likely to happen, and we have to provide a tenant-specific errors in these cases.

Having said that - that's our use case. I can imagine other reasons why making the load context available everywhere on the server-side is a good idea. As a matter of fact, it is already available everywhere, except in this one specific function. This PR fixes that.

[MichaelDeBoey]

@axel-xitaso What about getting the tenant info in the loader of the root? That way you would have it available too without needing this PR.

[ghost]

@MichaelDeBoey : Thanks for the suggestion. According to my tests, useLoaderData returns undefined in the root's catch and error boundaries (when rendered server-side, but interestingly, not on client navigations), but that's where we would (also) need this data. Or am I missing something?

Update: Remix event prints an error You cannot useLoaderData in an error boundary., so potentially it's a bug that it doesn't work for sever-side rendered top-level catch boundaries? In any case, we also need the tenant info in the global error boundary, so this PR is still necessary for us.

[axel-habermaier]

Are there any plans to get this merged?

If so, let me know and I'll rebase this PR onto the latest dev. Otherwise, I won't waste my time by doing so repeatedly without any indication whatsoever if this PR will ever get merged.

[tnightingale]

I can imagine other reasons why making the load context available everywhere on the server-side is a good idea. As a matter of fact, it is already available everywhere, except in this one specific function. This PR fixes that.

Just chiming-in to say I also found myself looking for load context in entry.server.tsx's handleRequest() today. I was surprised/disappointed to find that it's not available.

This PR fixes what appears to be an oversight in the API; I cannot think of a good reason to deliberately not share the load context with entry.server.tsx when it's available everywhere else that related to the request cycle. I would love to see this PR updated and accepted 👍

Thanks @axel-habermaier! :)

[ZipBrandon]

Adding my additional comment that this is important for me as well. I require an Apollo client in handleRequest which is being created in my server.ts and passed into the loadContext.

Personally, I cannot think of any reason not to expose the loadContext to the handleRequest.

[MichaelDeBoey]

@axel-habermaier Could you please rebase onto latest dev & resolve conflicts?

[axel-habermaier]

@MichaelDeBoey: Done.

[kiliman]

Despite my earlier concerns, I will +1 this PR. I think it will be useful for cases where loaders/actions fail. This will be the last opportunity to affect the response that depends on context.

[ZipBrandon]

These are odd failures in the🧪 Test / 🧪 Test OS: windows-latest for Node 14 and Node 18 which contrasts to Node 16 working.

[blorenz]

@mjackson Getting visibility of this since it did not make it in v1.6.0

[ZipBrandon]

Windows having that odd test failure again which was previously resolved by a force push.
-- 🧪 Test / 🧪 Test: (OS: windows-latest Node: 14) (pull_request)

[awwong1]

Discovered this PR while attempting to pass in Cloudflare Pages/Workers environment bindings (e.g. KV, DurableObjects) to the server's entry point.
Eager to see this change merged in and released.

[marwan38]

I've also discovered this PR while trying to pass in an instance of our logger from the express server into remix. I can get access to it just fine in actions and loaders, but, I want access to it in entry.server as well.

[sciutand]

Any movement on this?... we need this as well.
👍

[changeset-bot]

⚠️ No Changeset found

Latest commit: 269eb96

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

[sergiodxa]

I just fixed the conflict.

I also like this idea, if the team is ok with it I propose to merge it.

[marwan38]

PR needs to be updated after the 1.8.0 changes.

[MichaelDeBoey]

@axel-habermaier Can you please rebase onto latest dev, resolve conflicts & fix tests?

[github-actions]

This PR has been automatically closed because we haven't received a response from the original author 🙈. This automation helps keep the issue tracker clean from PRs that are unactionable. Please reach out if you have more information for us! 🙂

[sergiodxa]

😞

[MichaelDeBoey]

Re-opening this one as we were only a rebase away

@axel-habermaier Please rebase this branch onto latest dev & fix conflicts so we can merge this one.

@sergiodxa I think you can create a resubmission PR as well if you want to get this one merged.

[axel-habermaier]

@MichaelDeBoey: I opened this PR almost a year ago and already rebased it twice without it getting merged. Since I've completely lost all interest in Remix in the last couple of months, I won't put any more time into this PR.

Sorry for that, but since the changes are trivial, someone else should easily be able to open a new PR.

[MichaelDeBoey]

@sergiodxa If you want, please open a new PR to implement this

[benjamindulau]

Do you plan to fix this in a future release? Are someone willing to reopen this PR?
We are new to Remix (coming from React Router v6) and got stuck immediately after installation since we can't share our Apollo client instance with loaders and actions functions. This is confusing as getLoadContext is a documented feature but not enabled atm.

Are you tempering this because of this proposal on React Router remix-run/react-router#9564 which is likely to be brought back over to Remix?

In the mean time I think this is really a blocker for some users who need dependencies in their loaders or actions functions. Server side and client side.

[marwan38]

I was planning on recreating this PR but I’m on holiday atm. Perhaps I can get sometime soon to do it. We also need it but I’ve patched it on our side as a temp work around using npm patch package.

[benjamindulau]

@marwan38 Please bear with me since I'm not yet familiar with the Remix code base :-) But how did you patched it? I tried to apply a patch based on this PR, but I don't get how to pass the context to the <RemixServer /> component in the following server entry code:

// entry.server.tsx
import { PassThrough } from "stream";
import type { EntryContext } from "@remix-run/node";
import { Response } from "@remix-run/node";
import { RemixServer } from "@remix-run/react";
import isbot from "isbot";
import { renderToPipeableStream } from "react-dom/server";

const ABORT_DELAY = 5000;

export default function handleRequest(
  request: Request,
  responseStatusCode: number,
  responseHeaders: Headers,
  remixContext: EntryContext
) {
  return isbot(request.headers.get("user-agent"))
    ? handleBotRequest(
        request,
        responseStatusCode,
        responseHeaders,
        remixContext
      )
    : handleBrowserRequest(
        request,
        responseStatusCode,
        responseHeaders,
        remixContext
      );
}

function handleBotRequest(
  request: Request,
  responseStatusCode: number,
  responseHeaders: Headers,
  remixContext: EntryContext
) {
  return new Promise((resolve, reject) => {
    let didError = false;

    const { pipe, abort } = renderToPipeableStream(
      <RemixServer context={remixContext} url={request.url} />,
      {
        onAllReady() {
          const body = new PassThrough();

          responseHeaders.set("Content-Type", "text/html");

          resolve(
            new Response(body, {
              headers: responseHeaders,
              status: didError ? 500 : responseStatusCode,
            })
          );

          pipe(body);
        },
        onShellError(error: unknown) {
          reject(error);
        },
        onError(error: unknown) {
          didError = true;

          console.error(error);
        },
      }
    );

    setTimeout(abort, ABORT_DELAY);
  });
}

function handleBrowserRequest(
  request: Request,
  responseStatusCode: number,
  responseHeaders: Headers,
  remixContext: EntryContext
) {
  return new Promise((resolve, reject) => {
    let didError = false;

    const { pipe, abort } = renderToPipeableStream(
      <RemixServer context={remixContext} url={request.url} />,
      {
        onShellReady() {
          const body = new PassThrough();

          responseHeaders.set("Content-Type", "text/html");

          resolve(
            new Response(body, {
              headers: responseHeaders,
              status: didError ? 500 : responseStatusCode,
            })
          );

          pipe(body);
        },
        onShellError(err: unknown) {
          reject(err);
        },
        onError(error: unknown) {
          didError = true;

          console.error(error);
        },
      }
    );

    setTimeout(abort, ABORT_DELAY);
  });
}

Plus, our apollo client instances are not configured exactly the same between server and client, how would you pass it to loaders context on client side only components?

[benjamindulau]

Well, in the end I think that's what I need #4123
The problem for us is not really the fact that AppLoadContext is not passed to handleRequest (it should though) but more that with Remix App Server we can't define a getLoadContext function at all.

However, the question of having different instances passed to AppLoadContext instance depending on whether we are server-side or client-side remains... For instance, the Apollo Client factory takes a ssr: boolean option that should not be set to true client-side.

[gustavopch]

I need AppLoadContext in handleRequest as it seems the appropriate place to initialize Sentry on Cloudflare Pages given that I need the DSN environment variable from context.SENTRY_DSN.

This is what I want:

export default function handleRequest(request, respondeStatusCode, respondeHeaders, remixContext, loadContext) {
  Sentry.init({
    dsn: loadContext.SENTRY_DSN,
  })

  // ...
}

If there's a better way to do it though, please advise.