-
-
Notifications
You must be signed in to change notification settings - Fork 834
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
chore: better typings for DiscussionListState #3132
Conversation
@@ -2,6 +2,12 @@ import app from '../../forum/app'; | |||
import PaginatedListState, { Page } from '../../common/states/PaginatedListState'; | |||
import Discussion from '../../common/models/Discussion'; | |||
|
|||
export interface IRequestParams { |
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.
Could we enforce that if q
is present in filters, it should be the only key?
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.
Ooh, that's a good question. Let me have a play.
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 couldn't find a way.
One of the promising things was:
filter: { q: string } | { [key: string]: string } & { q?: never }
but this seemed to not work correctly, for some reason.
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 couldn't find a way.
One of the promising things was:
filter: { q: string } | { [key: string]: string } & { q?: never }but this seemed to not work correctly, for some reason.
Fair enough, let's keep things as is then
Changes proposed in this pull request:
Explicitly define function return types in DiscussionListState.
Defining these help to identify changes that inadvertently change the return type, and are this breaking.
Reviewers should focus on:
Are the requestParams typings correct?
Necessity
Confirmed
composer test
).Required changes: