[🐞] Redirects Not Working

JBischoff28 commented 7 months ago

Which component is affected?

Qwik City (routing)

Describe the bug

I am trying to redirect from within a routeAction$. Instead of redirecting the user, nothing happens at all. Sometimes the redirect will work, other times it will not. There has not been an identifiable pattern either. The issue has occured with both absolute and relative url paths. I have tried changing the redirect's status code, but that also had no effect.

This issue has been persistent in every project that I've used Qwik, so it is not project specific. I have also reached out on discord, and was told it was not an issue that I am making.

The following is a message that was sent to me by a Qwik team member: ok looks to me that the makeQDataPath function isn't happy with your redirect URL. And looking at it, it only allows redirecting to absolute paths in the same domain... https://github.com/BuilderIO/qwik/blob/396e98ebab748109818f19523dc185d12bdb6aa1/packages/qwik-city/middleware/request-handler/resolve-request-handlers.ts#L546-L556

Reproduction

npm create qwik@latest

Steps to reproduce

Run "npm create qwik@latest" to start a new quick project.
Create a basic form.
Create an empty route action and attach the route action to the Form's action prop.
Try to redirect to another page using requestEvent.redirect
Sometimes the redirect will work, other times it will not. I've been using relative paths when I am redirecting to another location within the app, and absolute paths when redirecting outside the app.

System Info

Nothing out of the ordinary. Use your standard development configs and you should get the same result.

Additional Information

No response

taichi221228 commented 6 months ago

@JBischoff28 See here. The status code may be the reason for the redirection.

PatrickJS commented 6 months ago

oh we can add a console warning or error in dev-only if the status codes aren't the redirecting kind

PatrickJS commented 6 months ago

I can add more errors etc. but the issue is this line then const url = new URL(href, 'http://localhost'); and how it's handled in makeQDataPath

function makeQDataPath(href: string) {
  if (href.startsWith('/')) {
    const append = QDATA_JSON;
    const url = new URL(href, 'http://localhost');

    const pathname = url.pathname.endsWith('/') ? url.pathname.slice(0, -1) : url.pathname;
    return pathname + (append.startsWith('/') ? '' : '/') + append + url.search;
  } else {
    return undefined;
  }
}

taichi221228 commented 6 months ago

Thanks for the answer! The logs may indeed be unfriendly, as nothing is output when an unsuitable URL is specified as well as a status code.

And looking at it, it only allows redirecting to absolute paths in the same domain...

I'm currently trying to resolve this issue. For reference, I checked the redirect function in Next.js and it does indeed support external links.

I'm almost up to a simple implementation, but I still haven't figured out how to redirect externally in SSR context.

PatrickJS commented 6 months ago

this is the nextjs redirect code https://github.com/vercel/next.js/blob/c26840909cf639aea181a01bb7b261bce43a5d22/packages/next/src/server/api-utils/index.ts#L59

/**
 *
 * @param res response object
 * @param [statusOrUrl] `HTTP` status code of redirect
 * @param url URL of redirect
 */
export function redirect(
  res: NextApiResponse,
  statusOrUrl: string | number,
  url?: string
): NextApiResponse<any> {
  if (typeof statusOrUrl === 'string') {
    url = statusOrUrl
    statusOrUrl = 307
  }
  if (typeof statusOrUrl !== 'number' || typeof url !== 'string') {
    throw new Error(
      `Invalid redirect arguments. Please use a single argument URL, e.g. res.redirect('/destination') or use a status code and URL, e.g. res.redirect(307, '/destination').`
    )
  }
  res.writeHead(statusOrUrl, { Location: url })
  res.write(url)
  res.end()
  return res
}

PatrickJS commented 6 months ago

~~so this means in qwik we just don't care~~

requestEv.status(statusCode || 307);
requestEv.headers.set('Location', adaptedLocation);
requestEv.getWritableStream().close();

export async function handleRedirect(requestEv: RequestEvent) {
  const isPageDataReq = requestEv.sharedMap.has(IsQData);
  if (isPageDataReq) {
    try {
      await requestEv.next();
    } catch (err) {
      if (!(err instanceof RedirectMessage)) {
        throw err;
      }
    }
    if (requestEv.headersSent) {
      return;
    }

    const status = requestEv.status();
    const location = requestEv.headers.get('Location');
    const isRedirect = status >= 301 && status <= 308 && location;
    if (isRedirect) {
      const adaptedLocation = makeQDataPath(location);
      if (adaptedLocation) {
        requestEv.headers.set('Location', adaptedLocation);
        requestEv.getWritableStream().close();
        return;
      } else {
        requestEv.status(200);
        requestEv.headers.delete('Location');
      }
    }
  }
}

update: ok actually the QDATA_JSON patch is probably needed

function makeQDataPath(href: string) {
  if (href.startsWith('/')) {
    const append = QDATA_JSON;
    const url = new URL(href, 'http://localhost');

    const pathname = url.pathname.endsWith('/') ? url.pathname.slice(0, -1) : url.pathname;
    return pathname + (append.startsWith('/') ? '' : '/') + append + url.search;
  } else {
    return href;
  }
}

PatrickJS commented 6 months ago

I'm not sure of the edge cases here and why the code was added this way https://github.com/QwikDev/qwik/pull/2622/files

taichi221228 commented 6 months ago

I tried a number of things with the Next.js code as a reference, but none of them worked.

requestEv.headers.set('Location', location);
requestEv.status(302);
requestEv.getWritableStream().close();

requestEv.redirect(302, location);
requestEv.getWritableStream().close();

None of these will simply work or will be blocked by CORS Policy on the browser. It is not clear why an error occurs on the browser side regarding redirects even though they are SSRs in the first place. Is it something to do with Qwik's Resumable?

PatrickJS commented 6 months ago

@JerryWu1234 do you want to work on this one?

JerryWu1234 commented 6 months ago

@JerryWu1234 do you want to work on this one?

please assign this to me

JerryWu1234 commented 5 months ago

Create an empty route action and attach the route action to the Form's action prop. Try to redirect to another page using requestEvent.redirect

Could you give me a small reproduction to make sure I understand correctly?

@JBischoff28 @PatrickJS

JerryWu1234 commented 5 months ago

production.tsx

import { component$ } from '@builder.io/qwik';
import { routeLoader$ } from '@builder.io/qwik-city';

export const useProductDetails = routeLoader$(async (requestEvent) => {
  const product = {
    name: 2222
  }
  console.log('Request headers:', requestEvent.request.headers);
  console.log('Request cookies:', requestEvent.cookie);
  console.log('Request url:', requestEvent.url);
  console.log('Request method:', requestEvent.method);
  console.log('Request params:', requestEvent.params);

  requestEvent.redirect(302, 'http://localhost:5173')
  return {
    productName: product.name
  };
});

export default component$(() => {
  const product = useProductDetails();
  return <div>Product name: {product.value.productName}</div>;
});

such as this ?

but I didn't capture this error

Nothing out of the ordinary. Use your standard development configs and you should get the same result.