Instead of having modules implicitly conform to PageModule, have the pages export default a data structure that explicitly implements PageModule instead.
That way we avoid using implicit typing on the pages and using modules as data structures.
Description
Description
Details
Details
| Subject | Repo | Branch | Lines +/- | |
|---|---|---|---|---|
| Chore: refactor page modules | marvin | master | +300 -269 |
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Declined | None | T176961 EOQ1 house cleaning | |||
| Resolved | • Niedzielski | T177236 Export pages as data implementing PageModule |
Event Timeline
Comment Actions
@Jhernandez, I looked into this and I'm not sure you'll like the approach I took using the wiki page as an example:
Before:
export const getInitialProps = (
params: Params = {}
): Promise<HttpResponse<Props>> =>
requestPage(
params.title === undefined
? { random: true }
: {
titlePath: params.title,
revision:
params.revision === undefined
? undefined
: parseInt(params.revision, 10)
}
).then(({ status, data }) => ({ status, data: { page: data } }));
export const Component = ({ page }: Props): JSX.Element => (
<App>
<Page
title={<ContentHeader titleHTML={page.titleHTML} />}
subtitle={page.descriptionText}
footer={<ContentFooter lastModified={page.lastModified} />}
>
<ContentPage sections={page.sections} />
</Page>
</App>
);After:
export const page: PageComponent<Params, Props> = {
getInitialProps(params = {}) {
return requestPage(
params.title === undefined
? { random: true }
: {
titlePath: params.title,
revision:
params.revision === undefined
? undefined
: parseInt(params.revision, 10)
}
).then(({ status, data }) => ({ status, data: { page: data } }));
},
Component({ page }: Props) {
return (
<App>
<Page
title={<ContentHeader titleHTML={page.titleHTML} />}
subtitle={page.descriptionText}
footer={<ContentFooter lastModified={page.lastModified} />}
>
<ContentPage sections={page.sections} />
</Page>
</App>
);
}
};And route becomes something like:
// ...
// Rename old PageModule to PageComponent
// ...
// Add new PageModule type for usage by Route.importModule().
export interface PageModule<
Params extends RouteParams | undefined = undefined,
Props = undefined
> {
page: PageComponent<Params, Props>;
}Conclusions:
- Less typing on getInitialProps and Component.
- Type checking is in the module itself.
- Still need a type for the module itself and doing the type checking for it within the module itself is not automatic (you have to add a dummy variable).
- Still using typing on route exports in api.ts to keep export typing convention.
Let me know if you'd like me to move forward with this approach, change it up, or decline this task. My inclination is not to pursue the approach I took.
Comment Actions
The current implementation may work better if a moduleof keyword is added:
type PageModule = moduleof "pages/..." | moduleof "pages/..." | ...
Comment Actions
@Niedzielski It is what it is if we want an explicit PageModule typing. It seems to me we should use default exports instead of a named page export, so:
const wiki: PageComponent<Params, Props> = {
getInitialProps(params = {}) {
//...
},
Component({ page }: Props) {
//...
}
};
export default wiki;The PageModule is as you mention, but with a default export instead:
export interface PageModule<
Params extends RouteParams | undefined = undefined,
Props = undefined
> {
default: PageComponent<Params, Props>;
}What do you think?
Comment Actions
Change 388069 had a related patch set uploaded (by Niedzielski; owner: Sniedzielski):
[marvin@master] Chore: refactor page modules
Comment Actions
I've taken a stab at it in this patch but omitted the PageComponent variables. Let me know what you think.