-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Conversation
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.
|
Agree w/ @pngwn. Explicit seems right. |
+1 to imported stores over props I haven't totally thought through |
Not going to pretend this is great code — there's literally a method called This doesn't yet add |
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 |
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 |
#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
andstatus
as props to$error.svelte
components. As a side-effect of this work it's very straightforward for error components to haveload
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 theload
function need access toerror
andstatus
?)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, sincehost
,path
,params
andquery
are inputs anderror
andstatus
are outputs. And I think it's confusing if thepage
store value sometimes has a different shape than thepage
object passed toload
.One alternative is to make
error
andstatus
available to all layout/error components. Another is to have a dedicated store(s), so that you could do this anywhere in your app:(If we did that, would we still pass
error
andstatus
as props to the error component?)Any others?
admin
Before submitting the PR, please make sure you do the following
Tests
pnpm test
and lint the project withpnpm lint
Changesets
pnpx changeset
and following the prompts