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

Nested error components #901

Merged
merged 24 commits into from
Apr 12, 2021
Merged

Nested error components #901

merged 24 commits into from
Apr 12, 2021

Conversation

Rich-Harris
Copy link
Member

@Rich-Harris Rich-Harris commented Apr 6, 2021

#519, despite the branch name (I decided to tackle #626 after this is done; should be easier that way round).

The implementation is a WIP but I'm almost at the point where we need to commit to some design decisions around #598 — would welcome any thoughts.

Presumably it makes sense to continue passing error and status as props to $error.svelte components. As a side-effect of this work it's very straightforward for error components to have load functions just like layout/page components, which they currently don't. I'm currently operating under the assumption that we allow that, unless people have strong objections. (It would mean that developers need to be careful to handle errors in their error components — it's not much help if the error page gets rendered because the user is offline, and promptly tries to fetch some data, so there is a footgun-related case for not allowing it. If we did, does the load function need access to error and status?)

Per #598, Sapper added an error property to the page object, allowing layout components to know if they were being rendered in the context of an error page. I do think there's value in layouts having access to this information (particularly the root layout), but I don't think it belongs on the page object, since host, path, params and query are inputs and error and status are outputs. And I think it's confusing if the page store value sometimes has a different shape than the page object passed to load.

One alternative is to make error and status available to all layout/error components. Another is to have a dedicated store(s), so that you could do this anywhere in your app:

import { error, status } from '$app/stores';

(If we did that, would we still pass error and status as props to the error component?)

Any others?


admin

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

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

@pngwn
Copy link
Member

pngwn commented Apr 6, 2021

I am in favour of the explicit import, it is more flexible and feels less magical. If we do this, I don't think we should pass them as props to error components.

If we removed error and status as props, would this be the last of the auto-passed props?

We used to get quite a few queries along the lines of 'how can i pass custom props to layouts/ error components'. It would be nice if we could just kill this pattern entirely in order to reduce confusion and framework magic. The explicit import is easier to reason about and in-line with Svelte's design principles.

load functions in error components would be very cool despite the footguns. I think they should be passed error and status.

@arxpoetica
Copy link
Member

Agree w/ @pngwn. Explicit seems right.

@benmccann
Copy link
Member

+1 to imported stores over props

I haven't totally thought through load in error components. I could see users wanting to sending info to the server to log that there's been an error. We'd want people to be careful not to nuke their error pages if there's an issue - the docs should probably show everything in load wrapped in a try/catch. On that note, are there some docs we should add to this PR? That's helped me understand the previous few PRs quite a lot

@Rich-Harris
Copy link
Member Author

Not going to pretend this is great code — there's literally a method called _rename_me — but the tests now pass. Will try and tidy it up a smidge and add some docs before merging.

This doesn't yet add status and error stores — that can happen later.

@Rich-Harris Rich-Harris marked this pull request as ready for review April 12, 2021 00:37
@Rich-Harris
Copy link
Member Author

ok, marked this ready for review. there's probably room to refactor some of this stuff, but i don't think that should stand in the way of other work

@Rich-Harris Rich-Harris merged commit 1007f67 into master Apr 12, 2021
@Rich-Harris Rich-Harris deleted the gh-626 branch April 12, 2021 20:26
@Rich-Harris
Copy link
Member Author

Not totally sure how those comments work tbh, I figured just documenting it near where the types were would be sufficient. If there's a better way to do it then I'm happy for us to do that

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.

None yet

6 participants