sveltejs / kit

web development, streamlined
https://kit.svelte.dev
MIT License
17.96k stars 1.8k forks source link

Redirects in `load` functions break when running client-side #5952

Closed WaltzingPenguin closed 1 year ago

WaltzingPenguin commented 1 year ago

Describe the bug

There appears to be no way to use the new redirect method and disable SSR globally.

Reproduction

  1. Create a new skeleton project
  2. Create src/routes/page.ts with the contents:
    
    import { redirect } from "@sveltejs/kit";

export function load() { throw redirect(307, "/about") }

3. Create `src/hooks.ts` with the contents:
```ts
import type { Handle } from '@sveltejs/kit'

export const handle: Handle = async ({ event, resolve }) => {
  return resolve(event, { ssr: false })
}
  1. Navigating to root now results in an error page, with the redirect class passed to it, instead of being redirected to "/about"

Repo: https://github.com/WaltzingPenguin/sveltekit-redirect-hooks

Logs

No response

System Info

System:
    OS: Windows 10 10.0.19043
    CPU: (12) x64 AMD Ryzen 5 1600 Six-Core Processor
    Memory: 7.50 GB / 15.95 GB
  Binaries:
    Node: 16.15.0 - C:\Program Files\nodejs\node.EXE
    Yarn: 1.22.10 - ~\AppData\Roaming\npm\yarn.CMD
    npm: 7.20.3 - C:\Program Files\nodejs\npm.CMD
  Browsers:
    Chrome: 104.0.5112.81
    Edge: Spartan (44.19041.1266.0), Chromium (104.0.1293.54)
    Internet Explorer: 11.0.19041.1566
  npmPackages:
    @sveltejs/adapter-auto: next => 1.0.0-next.64
    @sveltejs/kit: next => 1.0.0-next.413
    svelte: ^3.44.0 => 3.49.0
    vite: ^3.0.0 => 3.0.8

Severity

blocking an upgrade

Additional Information

No response

denovodavid commented 1 year ago

As a simple workaround I've done the following:

const location = '/login';
if (browser) return await goto(location);
else throw redirect(302, location);

This worked for me in browser and SSR, but yes ideally redirect() should work client side.

EDIT: may not work with links using sveltekit:prefetch

fev4 commented 1 year ago

I'm experiencing the same situation without disabling SSR:

// file: src/routes/+layout.ts

import {
  envelopeGuard,
  envelopeGuardRedirect,
} from '$lib/utils/guards';
import { accounts } from '$lib/stores/accountsStore';
import { get } from 'svelte/store';

export async function load({ url }) {
  const { pathname } = url;
  const accountsArray = get(accounts);
  const isRedirect = envelopeGuard({ pathname, accountsArray });
  const location = '/';
  if (isRedirect) {
    throw envelopeGuardRedirect(location);
  } else {
    return {
      key: pathname,
    };
  }
}
// file: src/lib/utils/guards.ts

import { redirect } from '@sveltejs/kit';
export function envelopeGuard({ pathname, accountsArray }) {
  if (
    accountsArray &&
    accountsArray.length === 0 &&
    pathname === '/create-envelope'
  ) {
    return true;
  }
  return false;
}

export const envelopeGuardRedirect = (location: string) =>
  redirect(302, location);

Then when triggering the guard and navigating to /create-envelope I get:

Uncaught (in promise) Redirect {status: 302, location: '/'}
WaltzingPenguin commented 1 year ago

As a simple workaround I've done the following:

const location = '/login';
if (browser) return await goto(location);
else throw redirect(302, location);

This worked for me in browser and SSR, but yes ideally redirect() should work client side.

Using goto as a workaround breaks as soon as a link has sveltekit:prefetch enabled. Hovering over the link will immediately redirect without requiring a click.

Rich-Harris commented 1 year ago

This is happening because of a failed instanceof Redirect check (SvelteKit's internals need to know if the object you threw from your load is a redirect). It's failing because the module that declares Redirect is being imported twice.

This line in client.js... https://github.com/sveltejs/kit/blob/eedc333bf6ee01fa5d99299b7d70ad3ceb653e8e/packages/kit/src/runtime/client/client.js#L19

...is being transformed by Vite into this:

import { HttpError, Redirect } from '/node_modules/.pnpm/@sveltejs+kit@1.0.0-next.413_svelte@3.49.0+vite@3.0.8/node_modules/@sveltejs/kit/src/index/private.js';

Later, when your code gets imported, it's turned into this:

import { base } from "/node_modules/.pnpm/@sveltejs+kit@1.0.0-next.413_svelte@3.49.0+vite@3.0.8/node_modules/@sveltejs/kit/src/runtime/app/paths.js";
import { redirect } from "/node_modules/.vite/deps/@sveltejs_kit.js?v=5bea743a";
export function load() {
  throw redirect(307, base + "/about");
}

That "/node_modules/.vite/deps/@sveltejs_kit.js?v=5bea743a" module is an esbuild bundle that includes the Redirect class. So the Redirect you have a hold of is totally different to the one imported by client.js.

We can prevent the esbuild stuff from happening by adding optimizeDeps.exclude: ['@sveltejs/kit'] to the Vite config, via the SvelteKit plugin. But we still end up with two copies of the module:

// from client.js
import { HttpError, Redirect } from '/node_modules/.pnpm/@sveltejs+kit@1.0.0-next.413_svelte@3.49.0+vite@3.0.8/node_modules/@sveltejs/kit/src/index/private.js';

// from index.js?v=e48ba282, which is imported by your `+page.ts` (it exports `redirect`)
import { HttpError, Redirect } from '/node_modules/.pnpm/@sveltejs+kit@1.0.0-next.413_svelte@3.49.0+vite@3.0.8/node_modules/@sveltejs/kit/src/index/private.js?v=e48ba282';

You'll notice it's the same URL, but with a cachebusting query parameter. I'm at the limits of my Vite knowledge, but maybe @bluwy or @dominikg would understand how to ensure that the query parameter is always present or always absent? I suspect it's related to the fact that we're importing client/start.js in the SSR'd HTML without going via transformIndexHtml.

benmccann commented 1 year ago

This is outside the portions of Vite I'm familiar with. Hopefully @bluwy or @dominikg will know. Leaving a few pointers here of what I did find. It looks to me like it has something to do with optimized deps as you mentioned:

https://github.com/vitejs/vite/blob/05712323b292492e9161a6ff7b20bfce43097dfb/packages/vite/src/node/optimizer/optimizer.ts#L115

Also, there's a method that can be used for removing it:

https://github.com/vitejs/vite/blob/05712323b292492e9161a6ff7b20bfce43097dfb/packages/vite/src/node/plugins/importAnalysis.ts#L493

As to whether it should always or never be there, I'm not sure.

madeleineostoja commented 1 year ago

This is also blocking an upgrade for me, and I have a simple use-case with SSR enabled but prerender disabled globally

+page.ts

export const load: Load = async ({ parent }) => {
  const { check, data } = await parent();

  if (!check) {
    throw redirect(302, data.path);
  }
};

check and data are dummy values to keep things simple, but that's the gist of what I'm doing. I redirect from / to another page if the user has a certain property (from db data fetched in +layout.ts). Rather than redirecting it hits my closest error boundary with a 500.

enyo commented 1 year ago

As a simple workaround I created a function for now:

import { browser } from '$app/env'
import { redirect } from '@sveltejs/kit'
import { goto } from '$app/navigation'

const redirectWorkaround: typeof redirect = (status, location) => {
  if (!browser) throw redirect(status, location)
  else {
    goto(location)
  }
}

EDIT: as @madeleineostoja pointed out: make sure you don't have sveltekit:prefetch enabled for these routes if you use this workaround.

madeleineostoja commented 1 year ago

as @WaltzingPenguin said, using goto as a workaround breaks with sveltekit:prefetch which will instantly trigger the redirect

bdanoit commented 1 year ago

It would be nice if Redirect also prevents child load functions that are awaiting the current load function from executing

Rich-Harris commented 1 year ago

@bdanoit agree, but please don't piggyback on unrelated issues — something like that deserves a new issue with a repro

bluwy commented 1 year ago

You'll notice it's the same URL, but with a cachebusting query parameter. I'm at the limits of my Vite knowledge, but maybe @bluwy or @dominikg would understand how to ensure that the query parameter is always present or always absent? I suspect it's related to the fact that we're importing client/start.js in the SSR'd HTML without going via transformIndexHtml.

IFAIA ?v= is usually added for prebundled deps only, and since we've excluded it from prebundling, plus it's in SSR, it's even weird that this is happening when it shouldn't.

I'm not sure yet if transformIndexHtml affects things here (I'm guessing unlikely), but this part could be the culprit from looking around:

https://github.com/sveltejs/kit/blob/eecbbfc6a4f13035163a2a6a3dd279f049325d32/packages/kit/src/vite/dev/index.js#L66

I might have to spin up locally to see what's up.

dummdidumm commented 1 year ago

With regards to Windows: Maybe this needs the same treatment as this? https://github.com/sveltejs/kit/blob/eecbbfc6a4f13035163a2a6a3dd279f049325d32/packages/kit/src/vite/dev/index.js#L290-L295

Rich-Harris commented 1 year ago

this part could be the culprit from looking around:

a hunch i haven't yet tested is that if we used the same runtime_base logic for client/start.js as we do for other things, everything would get resolved the same way.

The logic is basically this: if @sveltejs/kit is installed inside the project, do this...

import { start } from '/node_modules/@sveltejs/kit/src/runtime/client/start.js';

...otherwise (i.e. it's in a workspace root), use /@fs:

import { start } from '/@fs/path/to/repo/node_modules/@sveltejs/kit/src/runtime/client/start.js';

As I understand it that would match Vite's internal resolution logic more closely. As things stand we're doing some things with /node_modules/... and other things with /@fs, which might be at least part of the problem here.

benmccann commented 1 year ago

We should watch to see if this fixes it: https://github.com/vitejs/vite/pull/9730 and https://github.com/vitejs/vite/pull/9759

update from blu: it doesn't fix it

madeleineostoja commented 1 year ago

Is there any way to mark a route as “never prefetch” with goto as a workaround in the meantime? This has pretty thoroughly broken the large app I was halfway through migrating/refactoring so I’d be all for a hacky solution to duct tape it together for now

f-elix commented 1 year ago

I think calling goto in the +page.svelte file should work. Its far from ideal but if it works it can be a temporary workaround.

meteorlxy commented 1 year ago

You'll notice it's the same URL, but with a cachebusting query parameter. I'm at the limits of my Vite knowledge, but maybe @bluwy or @dominikg would understand how to ensure that the query parameter is always present or always absent?

@Rich-Harris This looks quite similar with https://github.com/vitejs/vite/issues/7621. We are using absolute fs paths to import modules, too, and the version hash made the same module to be different in the browser.

The reason is something like:

If the version hash is the root cause, I think https://github.com/vitejs/vite/pull/9730 might help. But @benmccann said no, so there might have some other issues here :thinking:

WaltzingPenguin commented 1 year ago

Is it worth a temporary patch to remove the use of instanceof from SvelteKit until the correct solution can be figured out with Vite? I have encountered a similar issue in the past and just used Symbols to work around it, instead of digging into Vite internals.

const redirect_symbol = Symbol.for('sveltekit:redirect')

export class Redirect {
    [redirect_symbol] = true;
    constructor(status, location) {
        this.status = status;
        this.location = location;
    }
}

export function is_redirect(obj) {
    return (redirect_symbol in obj)
}
f-elix commented 1 year ago

I agree, this bug is preventing me from migrating my app because it breaks key functionalities.

Rich-Harris commented 1 year ago

I've opened an issue on the Vite repo:

Would love to try and get it fixed properly rather than bodging in a workaround, as part of the reason for the instanceof checks is that TypeScript throws a hissy fit without them

madeleineostoja commented 1 year ago

Unless it's slated to be resolved very soon it might be worth throwing a warning in the docs, because it'll catch out anyone that tries to use throw redirect or throw error in load and leave them scratching their heads atm. This is the last major issue stopping me from pushing a fairly large app migrated to the 'new' sveltekit live so hoping it gets sorted upstream soon 🤞🏼

ivanhofer commented 1 year ago

Can we please also have the warning in the migration guide https://github.com/sveltejs/kit/discussions/5774 on the "Redirects" section? I was wasting quite a lot of time until I consulted the docs and found this issue.

Rich-Harris commented 1 year ago

Progress update — there's now an PR against Vite that fixes this issue: https://github.com/vitejs/vite/pull/9848

caesar-exploit commented 1 year ago

@Rich-Harris

My route looks like this: /api/hey/+server.ts

image

I can't get it to redirect at all, in a fetch I send a POST method, I don't use actions for this because I have to do an append with other values.

Conduitry commented 1 year ago

That's unrelated, and is the intended behavior. If you make a client-side fetch from the browser and that returns a redirect, the browser will not automatically cause the page making the request to redirect, but it will try to automatically follow the redirect for the fetch and will return the response from that. This is just how fetch works. If you need to navigate to a different page after a fetch, you need to do so yourself.

caesar-exploit commented 1 year ago

Now it makes sense.

Well then perform the fetch method POST in "no-cors" mode. The response simply does not return the redirect path.

image

image

image

aradalvand commented 1 year ago

@caesar-exploit A better approach would be to return the URL that your client should be redirected to as part of a JSON response. So in your RequestHandler, do:

return json({
    redirectTo: 'https://google.com',
});

And then on the client, just read that value and use SvelteKit's goto() or the native window.location or whatever to redirect.

caesar-exploit commented 1 year ago

That breaks my code

image

image

aradalvand commented 1 year ago

@caesar-exploit You need to use the json helper (import it from @sveltejs/kit):

return json({
    redirectTo: 'https://google.com',
});
caesar-exploit commented 1 year ago

So to specify a redirection, can I use: window.location.replace('url'); or do I have to do it differently?

I mean that my redirection must be successful. In some cases the pages block the redirections.

aradalvand commented 1 year ago

@caesar-exploit Yes you can use window.location.replace('url'). You can also just use SvelteKit's goto function (import from $app/navigation):

goto(yourFetchResponse.redirectTo);

Alternatively, you could do:

window.location = yourFetchResponse.redirectTo;

For the difference between window.location.replace(...) and window.location = ..., take a look at this Stack Overflow question.

caesar-exploit commented 1 year ago

I will do it thank you good man

aradalvand commented 1 year ago

Happy to help ;)

maximedupre commented 1 year ago

https://github.com/vitejs/vite/pull/9848

It's merged 🥹🎉

madeleineostoja commented 1 year ago

Just bumped all my deps and it's working perfectly for me, looks like this issue can be closed 🎉

WaltzingPenguin commented 1 year ago

Just bumped all my deps and it's working perfectly for me, looks like this issue can be closed 🎉

Retested the initial reproduction with these packages:

npmPackages:
    @sveltejs/adapter-auto: next => 1.0.0-next.67
    @sveltejs/kit: next => 1.0.0-next.445
    svelte: ^3.49.0 => 3.49.0
    vite: ^3.0.9 => 3.0.9

Error is still present.

timothycohen commented 1 year ago
npmPackages:
    @sveltejs/adapter-auto: next => 1.0.0-next.67
    @sveltejs/kit: next => 1.0.0-next.445
    svelte: ^3.49.0 => 3.49.0
    vite: ^3.0.9 => 3.0.9

Error is still present.

It's been merged to main, but not released.

madeleineostoja commented 1 year ago

Oh, then I have no idea why my env started working. Sorry for the false positive

madeleineostoja commented 1 year ago

Okay NOW it’s released, but in the 3.1.0-beta.1 so unless sveltekit wants to update a peer dep to a beta we’ll have to wait for a mainline 3.1 release.

It might be worth bumping vite now while leaving this open to track, since it’s such a breaking issue on sveltekit. Or at least mentioning it in the warning in the docs so users can override the version themselves

ShravanKumar-dev97 commented 1 year ago

There should be at update/warning update for users about vite new version(3.1.0-beta.1). yes, vite beta version casing GitHub action fail.

Graciasjr commented 1 year ago

Just do a server side redirect