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

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

Closed
emilioschepis opened this issue Apr 25, 2024 · 8 comments · Fixed by #9349 or #9372

Comments

@emilioschepis
Copy link

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

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 brophdawg11 closed this as not planned Won't fix, can't repro, duplicate, stale Apr 25, 2024
@kiliman
Copy link
Collaborator

kiliman commented Apr 25, 2024

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.

@brophdawg11
Copy link
Contributor

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
Copy link
Contributor

brophdawg11 commented Apr 26, 2024

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
Copy link
Contributor

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.

@rphlmr
Copy link
Contributor

rphlmr commented May 4, 2024

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.

@brophdawg11 brophdawg11 linked a pull request May 9, 2024 that will close this issue
@brophdawg11
Copy link
Contributor

This should be resolved by the introduction of defineLoader/defineAction in #9372. We'll have a prerelease out soon for validation.

@brophdawg11 brophdawg11 added awaiting release This issue has been fixed and will be released soon and removed bug:unverified labels May 9, 2024
@brophdawg11 brophdawg11 removed their assignment May 9, 2024
@brophdawg11 brophdawg11 removed the awaiting release This issue has been fixed and will be released soon label May 10, 2024
@brophdawg11
Copy link
Contributor

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