-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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(paginate): Return exact types from paginate() #8229
Conversation
🦋 Changeset detectedLatest commit: 65e3007 The changes in this PR will be included in the next version bump. 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 |
@@ -57,7 +57,7 @@ declare module 'astro:assets' { | |||
}; | |||
|
|||
type WithRequired<T, K extends keyof T> = T & { [P in K]-?: T[P] }; | |||
type Simplify<T> = { [KeyType in keyof T]: T[KeyType] }; | |||
type Simplify<T> = { [KeyType in keyof T]: T[KeyType] } & {}; |
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 a small fix, eventually when we refactor types, we should probably have a "internal-type-utils" file somewhere with this and all the other nonsense I add
paginate: generatePaginateFunction(route), | ||
// Q: Why the cast? | ||
// A: So users downstream can have nicer typings, we have to make some sacrifice in our internal typings, which necessitate a cast here | ||
paginate: generatePaginateFunction(route) as PaginateFunction, |
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 think there is a way to make this work without the cast, but I couldn't for my life figure it out. I think the disconnect between the internal implementation and what the user actually uses makes it very hard to do in one type
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.
Seems like a reasonable tradeoff!
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.
Looks great, approved!
Would you mind closing out #8048 and the related issue after merging this since they overlap?
Changes
paginate()
currently returns a naiveGetStaticPathsResult
which override the actual types it returns, this PR makes it so it actually returns what is being inputted, which because of the fact that it allows you to inject params and props, made this strangely hard. Maybe there's an easier way to do it, but I couldn't figure it outThis is similar to #8048, but it doesn't require user to manually type
PaginateFunction
This PR also fixes #7159, mostly by accident
Testing
Tested manually
Docs
N/A. Docs document the return values of paginate(), so this really just makes it more accurate to what's currently described