fix: adjust fetch error message on the server #8551
Conversation
🦋 Changeset detectedLatest commit: b18c839 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| if (typeof info === 'string' && !/^\w+:\/\//.test(info)) { | ||
| await Promise.resolve(); // allows people to use malformed fetch urls in `{#await ..}` (native fetch error is swallowed by those) |
There was a problem hiding this comment.
This came out of #8536 (comment) , doing {#await fetch('/foo')}.. worked previously, because the error was swallowed.
Either we treat this as a potential breaking change because this worked previously; also it may be deliberate to rely on this behavior on the server to save some code ("yeah this will fail but I don't care, it will work on the client then"). Or we treat it as a bug that this ever worked and revert the 2nd commit of this PR.
There was a problem hiding this comment.
I'm in favour of reverting — this behaviour seems much too magical to me
There was a problem hiding this comment.
What's magical about it for you? That this relies on specific behavior of #await? I'm not sure if that's the case but maybe this also works if we omit that line if the function is marked as async? Would that be too magical, too?
Do you think it's edge case enough that we can mark this as "this merely uncovered a bug in your code and is not a breaking change"?
There was a problem hiding this comment.
Rule 1: don't swallow errors.
If my intent is to have an await that only works in the browser, then that's the code I should write. It's far more likely that someone simply didn't understand the mechanics of await and fetch, and this kind of benevolent interference prevents them from developing that understanding (while causing confusion to someone who understands why it shouldn't work).
This definitely qualifies as 'we found a bug in your code', yeah
| throw new Error( | ||
| `Cannot use relative URL (${info}) with global fetch — use \`event.fetch\` instead: https://kit.svelte.dev/docs/web-standards#fetch-apis` | ||
| `Cannot use relative URL (${info}) with global fetch on the server. If this is called inside a \`load\` function, use \`event.fetch\` instead: https://kit.svelte.dev/docs/web-standards#fetch-apis` |
There was a problem hiding this comment.
You can use event.fetch inside hooks and request handlers as well. But I'm not sure that changing the text to 'If this is called inside a load function, a request handler or a hook,' makes sense because apart from init code, all server-only code runs in the context of responding to an event.
Not 100% sure what the solution is but I think we need to keep brainstorming
There was a problem hiding this comment.
The primary goal is to give people an error message that unblocks them/gives them pointers what to do instead.
What about
| `Cannot use relative URL (${info}) with global fetch on the server. If this is called inside a \`load\` function, use \`event.fetch\` instead: https://kit.svelte.dev/docs/web-standards#fetch-apis` | |
| `Cannot use relative URL (${info}) with global fetch on the server. If this coming from a fetch call inside a component, ensure that it is not run eagerly on the server, for example by wrapping the call inside \`onMount\`. Else, use \`event.fetch\` instead: https://kit.svelte.dev/docs/web-standards#fetch-apis` |
There was a problem hiding this comment.
We can intercept eager fetch calls during component rendering. What about doing this, and keeping the existing error message for other cases?
+// src/runtime/server/page/render.js
+const { fetch } = globalThis;
+globalThis.fetch = () => {
+ throw new Error(
+ 'Cannot call `fetch` eagerly during SSR — put your `fetch` calls inside `onMount` or `load` instead'
+ );
+};
rendered = options.root.render(props);
+globalThis.fetch = fetch;There was a problem hiding this comment.
Oooh yeah that's nice, will adjust (though not to what you have there, that's just too breaking for people who still eagerly have it inside #await)!
There was a problem hiding this comment.
Is it though? People who are doing that shouldn't be, and it's really a bug that it's allowed. If I was making server-side requests that had no effect but were costing me actual dollars, I wouldn't consider it a breaking change if my framework stopped me from doing that
There was a problem hiding this comment.
The related issue has three thumbs up and one more person saying they had to adjust all their #await imports, I'm not sure if this kind of rigourus throwing of an error still not counts as "not a breaking change", so I'm against that. I'm all for emitting a warning though, and make it an error in 2.0.
(Or maybe we have some kind of async server components then and it is allowed 😅 )
|
@Rich-Harris is this good to merge for you? If so, could you approve it? |
probably closes #8536, but let's wait a little for the OP to answer my question first.
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm testand lint the project withpnpm lintandpnpm checkChangesets
pnpm changesetand following the prompts. Changesets that add features should beminorand those that fix bugs should bepatch. Please prefix changeset messages withfeat:,fix:, orchore:.