remix icon indicating copy to clipboard operation
remix copied to clipboard

Single fetch `response` parameter for loader is undefined if no default export exists in the route

Open emilioschepis opened this issue 1 year ago • 6 comments

Reproduction

https://stackblitz.com/edit/remix-run-remix-x55her?file=app%2Froutes%2Fno-default.tsx

System Info

System:
    OS: Linux 5.0 undefined
    CPU: (8) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
    Memory: 0 Bytes / 0 Bytes
    Shell: 1.0 - /bin/jsh
  Binaries:
    Node: 18.18.0 - /usr/local/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 10.2.3 - /usr/local/bin/npm
    pnpm: 8.15.6 - /usr/local/bin/pnpm
  npmPackages:
    @remix-run/dev: * => 2.9.1 
    @remix-run/node: * => 2.9.1 
    @remix-run/react: * => 2.9.1 
    @remix-run/serve: * => 2.9.1 
    vite: ^5.1.0 => 5.2.10

Used Package Manager

npm

Expected Behavior

The response parameter should always be a ResponseStub, even in resource routes without a default export.

Actual Behavior

Visiting a resource route (in the stackblitz: app/routes/no-default.tsx) results in the response starting as undefined and an error being printed to the console:

The following error is a bug in Remix; please open an issue! https://github.com/remix-run/remix/issues/new
Error: Expected a Response to be returned from queryRoute

emilioschepis avatar Apr 25 '24 13:04 emilioschepis

Resource routes don't need a response stub because they return actual Response instances that contain a status and headers - so you just set them directly. No need for a stub when you have the real thing!

The response stub concept for single fetch is needed because those loaders don't return a Response - they return data that is merged with other loader data into a Response, so we need to give you some abstraction to handle status/headers.

brophdawg11 avatar Apr 25 '24 14:04 brophdawg11

I think this is a breaking change with the new Single Fetch feature.

Before, your resource routes could return a naked object and Remix would automatically convert that to a JSON response.

Now, if you return a naked object from a resource route, you will get that error message.

It's not technically a Remix error. You should check to see if unstable_singleFetch is true and then let the developer know that that resource routes can no longer return a naked object. They can still use the json helper, since it creates a new Response for you.

kiliman avatar Apr 25 '24 15:04 kiliman

yeah, we should probably add a section to the docs to make this more clear, but this section in the docs applies to all loaders/actions - including resource routes.

Naked objects returned from loader and action functions are no longer automatically converted into a JSON Response and are serialized as-is over the wire

Resource routes in particular don't do anything special for single fetch - and since there's no longer an automatic conversion we don't have a Response to send back anymore.

brophdawg11 avatar Apr 26 '24 13:04 brophdawg11

We decided we are going to keep this behavior in v2 for backwards compatibility and to make adoption easier. We'll detect naked objects from resource routes only and convert them to json() responses and log a deprecation warning so you know to go update it. In the next major version of Remix this automatic conversion won't happen.

brophdawg11 avatar Apr 26 '24 17:04 brophdawg11

Please see the dev branch docs for details on resource routes in single fetch with this change. These changes will be available in the next release.

Keeping this issue open as we are going to get the response type ambiguity cleaned up - this will likely require a new V3_LoaderFunctionArgs type you'll need to switch to when adopting single fetch.

brophdawg11 avatar May 02 '24 12:05 brophdawg11

I hope we can keep ResponseStub everywhere because it helps to build simple things like that:

https://supabase.com/docs/guides/auth/server-side/creating-a-client?queryGroups=framework&framework=remix&queryGroups=environment&environment=remix-action#creating-a-client

export function getSupabaseServerClient(
  request: Request,
  response: ResponseStub,
) {
  const cookies = parse(request.headers.get("Cookie") ?? "");

  return createServerClient(env.SUPABASE_URL, env.SUPABASE_ANON_KEY, {
    cookieOptions,
    cookies: {
      get(key) {
        return cookies[key];
      },
      set(key, value, options) {
        response.headers.append("Set-Cookie", serialize(key, value, options));
      },
      remove(key, options) {
        response.headers.append("Set-Cookie", serialize(key, "", options));
      },
    },
  });
}

For this specific example, it could be solve with middleware, though. At least, returning something null is a solution, so... Maybe it's fine.

rphlmr avatar May 04 '24 16:05 rphlmr

This should be resolved by the introduction of defineLoader/defineAction in https://github.com/remix-run/remix/pull/9372. We'll have a prerelease out soon for validation.

brophdawg11 avatar May 09 '24 15:05 brophdawg11

2.9.2 is released - https://github.com/remix-run/remix/releases/tag/remix%402.9.2

brophdawg11 avatar May 10 '24 19:05 brophdawg11