-
-
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
[feat] add App.PageData type #6226
Conversation
🦋 Changeset detectedLatest commit: 7fe3c4c The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
I was thinking we could add a
That would solve the O(n^2) problem and also ensure that the parent-child relationships were correct (which the current naive approach doesn't, since |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just had some minor comments, but to the extent I was able to understand this (it's brain-bending stuff 😅) it looks awesome. Very excited for this one
let server_load; | ||
let errors; | ||
function process_node(node, outdir, is_page, all_pages_have_load = true) { | ||
const params = `${is_page ? 'Route' : 'Layout'}Params`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a side-note, probably for a separate PR, but I've been wondering if we should just always spell out the params type, i.e.
PageLoad<{ id: string }>
rather than
PageLoad<RouteParams>
It's obviously more repetitive, but it means that if you hover over the params
object you see this...
...instead of this, which is less useful:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's a good idea yeah, let's do this in a separate PR.
Closes #5951
Marking as draft because the "layout data is required if one of the pages doesn't have a load function" check isn't implemented yet. If have an idea about storing the layout nodes in a map, which is updated by its pages, which also solves the O(n^2) problem, but my head needs a break so I'll revisit it later.
This all has gotten pretty sophisticated, which is why I'm also inclined to create some tests for it.
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm test
and lint the project withpnpm lint
andpnpm check
Changesets
pnpm changeset
and following the prompts. All changesets should bepatch
until SvelteKit 1.0