-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
refactor routing logic #211
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
Conversation
}; | ||
|
||
branch = await Promise.all( | ||
[[() => this.layout, () => {}], ...route.parts].map(async ([loader, get_params], i) => { |
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.
I don't really get what this is doing. Why do we need a loader
instead of just accessing this.layout
directly? Why is get_params
always returning {}
?
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.
Why do we need a
loader
instead of just accessingthis.layout
directly
because route.parts
is an array of [loader, get_params]
arrays, but the layout component is excluded from the manifest (because it's always present) so needs to be inserted before other components in a consistent way, and this was the quickest way to get it to behave
|
||
const { default: component, preload } = await loader(); | ||
|
||
const params = get_params ? get_params(match) : {}; |
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.
do we ever have to worry about get_params
not being defined here?
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.
yes. it's only included for page/layout components that have [dynamic]
routes
A few minor TODOs remaining here and there but I think it makes sense to merge this for now |
This code was confusing the hell out of me. Still lots to do but this should be a lot friendlier. Essentially it adds two singletons,
router
andrenderer
—router
selects routes given anhref
and interacts with thehistory
API, whilerenderer
is responsible for rendering a selected route (which includes loading its code and runningpreload
).