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

Navigation HTTP error handling #12399

Open
hopperelec opened this issue Jun 24, 2024 · 13 comments · May be fixed by #12579
Open

Navigation HTTP error handling #12399

hopperelec opened this issue Jun 24, 2024 · 13 comments · May be fixed by #12579

Comments

@hopperelec
Copy link

Describe the problem

I have a "form" like this:

<script>
let value = "";
</script>

<input type="text" placeholder="Game ID..." bind:value/>
<button type="button" on:click={async () => await goto(`/game/${value}/`} disabled={!value}>Join game</button>

Currently, if some error occurs while trying to join the game (e.g: if the game doesn't exist), the error will be shown by the /game/[gameId]/ route, meaning the user needs to navigate back. Ideally, the error message would be displayed as part of the form.

Describe the proposed solution

I'm not sure exactly how the flow from beforeNavigate to onNavigate to afterNavigate works, but I assume SvelteKit would be aware of errors during onNavigate in which case $page.error should be set before the onNavigate callback is called and then onNavigate should have a cancel method similarly to beforeNavigate. Alternatively, there could be a dedicated onNavigationError

Alternatives considered

Have a route which checks if a game exists, then call that before calling goto. However, this means I need to check if the game exists twice, which isn't ideal.

Importance

would make my life easier

Additional Information

No response

@eltigerchino
Copy link
Member

Would preload fit your use case? For example:

<script>
import { preload, goto } from '$app/navigation';

let value =  '';

async function customGoto(href) {
  try {
    await preload(href);
  } catch (error) {
    alert(error.message)
  }
  await goto(href);
}
</script>

<input type="text" placeholder="Game ID..." bind:value/>
<button type="button" on:click={async () => await customGoto(`/game/${value}/`} disabled={!value}>Join game</button>

@hopperelec
Copy link
Author

hopperelec commented Jun 24, 2024

I hadn't actually seen preload so thanks for showing me this! However, I couldn't get it working.

await preload doesn't throw an error. This seems like it should be the working customGoto

async function joinGame(href: string) {
	const preloadResult = await preloadData(href);
	if (preloadResult.type === "loaded" && preloadResult.status !== 200) {
		alert(preloadResult.status);
	} else {
		await goto(href);
	}
}

However, it doesn't work. preload.status always seems to be 200. If I put a console.log(preloadResult) and look in the console, I see this which clearly shows that the page is throwing a 403 error, but preloadResult shows 200.
image
If I look in the network tab, I can see that no 403 status is actually being returned for any requests and that the client is only being informed of the error via the __data.json response body.

{
    "type": "data",
    "nodes": [
        {
            "type": "skip"
        },
        {
            "type": "skip"
        },
        {
            "type": "error",
            "error": {
                "message": "You do not have access to this game!"
            },
            "status": 403
        }
    ]
}

So, based on all this, I'm guessing that this method would only work for errors which would outright prevent a goto from working, not for errors handled by the app.

@eltigerchino
Copy link
Member

eltigerchino commented Jun 30, 2024

Sorry for getting back to you late! I found some time to play around and can confirm there's no obvious way to detect errors because it's been explicitly excluded from the preloadData function return.

const result = await _preload_data(intent);
if (result.type === 'redirect') {
return {
type: result.type,
location: result.location
};
}
const { status, data } = result.props.page ?? page;
return { type: result.type, status, data };
}

Here's a "workaround" in the meantime: you can check the data property of the returned result and if there are no keys, then it's probably an error (or the load function just doesn't return data on success).

import { preloadData, goto } from '$app/navigation';

async function attemptGoto (pathname) {
	const result = await preloadData(pathname);

	// load function threw an error
	if (result.type === 'loaded' && Object.keys(result.data).length === 0) {
		alert('there was an error so we do not navigate');
		return;
	}

    // load function threw a redirect
	if (result.type === 'redirect') {
		await goto(result.location);
		return;
	}

    // load function was successful
	await goto(pathname);
}

demo

@eltigerchino
Copy link
Member

@Rich-Harris should preloadData know about thrown errors from load functions or should it primarily be used to load the code / data for the next page?

@eltigerchino eltigerchino added needs-decision Not sure if we want to do this yet, also design work needed router labels Jun 30, 2024
@hopperelec
Copy link
Author

hopperelec commented Jun 30, 2024

That's an interesting workaround. It doesn't work if the target route uses a layout which returns data but, to get around that, I can pass the number of keys (hard-coded) in the layout of the target route to customGoto and check against that. Ideally, though, I'd be able to show the user the specific error message.

@Rich-Harris
Copy link
Member

It does feel like result.type should be 'loaded' | 'redirect' | 'error' rather than 'loaded' | 'redirect'. This feels like an oversight. Would fixing that be a breaking change, do we think?

@eltigerchino eltigerchino added error handling and removed router needs-decision Not sure if we want to do this yet, also design work needed labels Jul 14, 2024
@carmas123
Copy link

This is a very simple reproduction:

https://stackblitz.com/edit/sveltejs-kit-template-default-7retxz?file=src%2Froutes%2F%2Bpage.svelte

@hmnd
Copy link
Contributor

hmnd commented Jul 25, 2024

Unfortunately, @eltigerchino's workaround doesn't quite work, since preloadData still returns the layout data even if it couldn't load the page data. I've encountered this issue using {#await} in a shallow loaded dialog. It looks like the best workaround for now is to explicitly check if a particular key exists on the result.data object.

@eltigerchino eltigerchino linked a pull request Aug 15, 2024 that will close this issue
8 tasks
@eltigerchino
Copy link
Member

eltigerchino commented Aug 15, 2024

I've drafted a PR for this fix. I think we can agree it's not a breaking change since it never worked correctly in the first place (status always returned 200 even on errors). The new return type should look something like below:

export function preloadData(href: string): Promise<{
	type: 'loaded';
	status: number;
	data: Record<string, any>;
} | {
	type: 'redirect';
	location: string;
} | {
	type: 'error';
	status: number;
	error: App.Error;
}>;

Any thoughts on this?

@hopperelec
Copy link
Author

hopperelec commented Aug 15, 2024

Awesome! This was an existing issue so it might be worth creating a separate issue/PR for this, but you showing the type has made me notice that the status is missing is there's a redirect, which seems a bit pointless. For one, if the status is always provided then it makes any logic dependent on the status a lot simpler since it doesn't need to check the type first. But also, there are many redirect HTTP status codes (300 - 399) which could be used by developers to quickly indicate information which the client might want to know before checking the location.

@eltigerchino
Copy link
Member

eltigerchino commented Aug 15, 2024

the status is missing if there's a redirect, which seems a bit pointless. For one, if the status is always provided then it makes any logic dependent on the status a lot simpler since it doesn't need to check the type first. But also, there are many redirect HTTP status codes (300 - 399) which could be used by developers to quickly indicate information which the client might want to know before checking the location.

Currently, the server doesn't return the status property in the __data.json payload if it's a redirect, so we would need to change that if we decide to standardise this.

@hopperelec
Copy link
Author

Hm, that's a bit annoying, but would I be correct in saying that wouldn't be a breaking change since it would just mean adding (not removing/changing) a property? Or is the keyset used for validation or something?

@eltigerchino
Copy link
Member

eltigerchino commented Aug 16, 2024

Hm, that's a bit annoying, but would I be correct in saying that wouldn't be a breaking change since it would just mean adding (not removing/changing) a property? Or is the keyset used for validation or something?

It shouldn't be. I haven't heard any objections around returning the status for redirects yet so I tried adding it. The new interface looks something like this:

	(
		| {
				type: 'loaded';
				data: Record<string, any>;
		  }
		| {
				type: 'redirect';
				location: string;
		  }
		| {
				type: 'error';
				error: App.Error;
		  }
	) & {
		status: number;
	}

@eltigerchino eltigerchino added this to the 3.0 milestone Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants