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

Explicit fallthrough #3217

Merged
merged 17 commits into from
Jan 10, 2022
Merged

Explicit fallthrough #3217

merged 17 commits into from
Jan 10, 2022

Conversation

nhe23
Copy link
Contributor

@nhe23 nhe23 commented Jan 5, 2022

This PR makes fallthrough explicit. Only if {fallthrough: true} is returned by a load function or an endpoint the fallthrough mechanism is triggered. This way it is more clear when fallthrough is applied.
Fixes #2440 by allowing the same fallthrough logic that is used for page components.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

@changeset-bot
Copy link

changeset-bot bot commented Jan 5, 2022

🦋 Changeset detected

Latest commit: ef33ab1

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 5, 2022

❌ Deploy Preview for kit-demo failed.

🔨 Explore the source changes: 05ebc5a

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

@Rich-Harris
Copy link
Member

Thank you. My first reaction was that this could be confusing — it's much more likely that you'd simply forget to return something from load than that you'd want a layout to fall through.

But you could easily argue that the same is true for pages and endpoints. I've definitely been caught out by that once or twice (i.e. 404s that I wasn't expecting).

So I was wondering if we shouldn't make it explicit — make the return value required (i.e. load and endpoint handlers must return something), and make fallthroughs explicit:

<script context="module">
  async function load({ params }) {
    if (params.foo !== 'okay') {
      return { fallthrough: true };
    }
    return {};
  }
</script>

Thoughts?

@nhe23
Copy link
Contributor Author

nhe23 commented Jan 7, 2022

The implicit fallthrough when nothing is returned is indeed a bit confusing. I'll change that to your proposed solution. When fallthrough is returned all other properties won't be considered. Should this be reflected in the return type of load and endpoints? So for load either the normal load output is returned or {fallthrough:true}. The type could look like this:

export interface LoadOutput<
	Props extends Record<string, any> = Record<string, any>,
	Stuff extends Record<string, any> = Record<string, any>
> {
	status?: number;
	error?: string | Error;
	redirect?: string;
	props?: Props;
	stuff?: Stuff;
	maxage?: number;
} | {fallthrough: boolean }

Or is it enough to point this behavior out in the docs?

@dummdidumm
Copy link
Member

It definitely should be reflected in the types 👍

@Rich-Harris
Copy link
Member

It should be | { fallthrough: true } rather than | { fallthrough: boolean } I think (there's no reason you'd have false), but other than that yeah. Similarly for RequestHandler:

export interface RequestHandler<
Locals = Record<string, any>,
Input = unknown,
Output extends DefaultBody = DefaultBody
> {
(request: ServerRequest<Locals, Input>): MaybePromise<void | EndpointOutput<Output>>;
}

@nhe23 nhe23 changed the title Layout fallthrough Explicit fallthrough Jan 8, 2022
@nhe23
Copy link
Contributor Author

nhe23 commented Jan 8, 2022

Fallthrough is now explicit as suggested by @Rich-Harris. In order to reflect this in the types I added an Either type to the helpers, because with Union Types only the values that the types have in common are directly accessible. So it would have been necessary to first check the returned type before accessing returned properties.

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.

Thanks, this is looking good — just a couple of small things I found

documentation/docs/01-routing.md Outdated Show resolved Hide resolved
packages/kit/src/runtime/server/page/load_node.js Outdated Show resolved Hide resolved
Co-authored-by: Rich Harris <hello@rich-harris.dev>
@netlify
Copy link

netlify bot commented Jan 10, 2022

✔️ Deploy Preview for kit-demo canceled.

🔨 Explore the source changes: ef33ab1

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

@Rich-Harris
Copy link
Member

thank you!

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.

Layout fallthrough
3 participants