-
Notifications
You must be signed in to change notification settings - Fork 1
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: refactor async app routes to factory #2550
Conversation
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.
Thanks for tackling this, quite a bit of an uplift! 🥇
I've left some comments about the use of the defaultPageFactory
that apply to most pages refactor - we'll need to make sure:
- that the Next app page params (version_id, facility_id, etc) align with the props of the page components, otherwise the page factory won't know what to map
- that we handle the
seachParams
case in a consistent way where they're needed (either flatten them, or add them in their own prop)
<FacilityReviewForm | ||
version_id={params.version_id} | ||
facility_id={params.facility_id} | ||
/> |
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.
The defaultPageFactory
uses generic typing to forward the type of the params
object to the component props, so we can use it right out of the box here too. they just need to be
export default defaultPageFactory(FacilityReviewForm);
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.
Also, how about building a syncPageFactory
, or have an async=false
optional param in that default factory?
</Suspense> | ||
); | ||
} | ||
export default defaultPageFactory(OperationsPage); |
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.
the defaultPageFactory
doesn't forward the searchParams
yet, we'll need to add them if they're needed
</Suspense> | ||
); | ||
} | ||
export default defaultPageFactory(AdditionalReportingDataPage); |
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.
the AdditionalReportingDataPage
component expects a versionId
but Next is setup to set version_id
.
I suggest making the AdditionalReportingDataPage
component (and all the other ones) extend the HasReportVersion
or HasFacilityId
interfaces where applicable, to guarantee compatibility
import defaultPageFactory from "@bciers/components/nextPageFactory/defaultPageFactory"; | ||
import ActivityPage from "@reporting/src/app/components/activities/ActivityPage"; | ||
|
||
export default defaultPageFactory(ActivityPage); |
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.
the defaultPageFactory
won't know how to resolve the ActivityPage props from the params
object.
This one is probably the most complex, we can maybe determine how we want to handle the params
vs searchParams
for every page to be consistent
No card, just cleanup.
🚀 Impact:
bciers/libs/components/src/nextPageFactory/defaultPageFactory.tsx
📝 Notes:
defaultPageFactory.tsx
Ticket