Skip to content

Conversation

@benmccann
Copy link
Member

We were setting the type of the user-provided handle function as InternalHandle, which is not correct and was making it ward to work with

@changeset-bot
Copy link

changeset-bot bot commented Jan 26, 2022

🦋 Changeset detected

Latest commit: 7419aea

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit Patch

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

@netlify
Copy link

netlify bot commented Jan 26, 2022

✔️ Deploy Preview for kit-demo canceled.

🔨 Explore the source changes: 7419aea

🔍 Inspect the deploy log: https://app.netlify.com/sites/kit-demo/deploys/61f16339d09ba400082af852

Copy link
Member

@Rich-Harris Rich-Harris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, looking at the code, i think this does exist for a reason. lemme come back to this

@benmccann
Copy link
Member Author

my motivation for this was to move AMP validation to a hook, but it depended on this. I pushed a branch for that here: https://github.com/sveltejs/kit/tree/amp-hook

I couldn't figure out how to get the types to work otherwise. I could probably do it by casting the types and/or using @ts-expect-error, but it felt rather ugly to try that

@Rich-Harris
Copy link
Member

so, here's the problem. if a lambda is rendering a page foo that depends on endpoint bar, and the code for bar isn't in the lambda, then instead of returning a 404 response, respond needs to signal that it needs to fall back to a real HTTP request. It only guarantees to generate a response if the request came from the user, rather than from load:

// if this request came direct from the user, rather than
// via a `fetch` in a `load`, render a 404 page
if (!state.initiator) {
const $session = await options.hooks.getSession(event);
return await respond_with_error({
event,
options,
state,
$session,
status: 404,
error: new Error(`Not found: ${event.url.pathname}`),
ssr
});
}

at the same time, this does mean that the handle signature is wrong. since there's no guarantee that resolve will return something, it'd be possible to write buggy code:

export async function handle({ event, resolve }) {
  const response = resolve(event);
  response.headers.set('x-vegetable', 'potato'); // errors — response is undefined
  return response;
}

i think that's the only place where we're using the lack of a response that way (though at the same time i'm pretty sure the distinction between Handle and InternalHandle predates the server-side code-splitting stuff, so maybe not?). Perhaps we could solve it some other way, e.g. a special header on the response, or a single Response object that both sides have a reference to and that could be used as a sentinel value.

@benmccann
Copy link
Member Author

Ok. I think I've fixed that by moving where the real HTTP request occurs

@Rich-Harris
Copy link
Member

oh, nice — nailed it. this is way better

@Rich-Harris
Copy link
Member

cloudflare app is failing to deploy but that has to be unrelated. choosing to ignore it

@Rich-Harris Rich-Harris merged commit b3d3f5c into master Jan 26, 2022
@Rich-Harris Rich-Harris deleted the internal-handle branch January 26, 2022 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants