chore(gatsby): Migrate utils/page-data to TypeScript #23436
Conversation
| export const write = async ( | ||
| { publicDir }: IPageDataContext, | ||
| { componentChunkName, matchPath, path }: IGatsbyPage, | ||
| result: any |
There was a problem hiding this comment.
Here, I was not really sure what is the shape of the result argument so I judge any being safer. But feel free to indicate a more precise typing for this line, if existing.
| export default { | ||
| fixedPagePath, | ||
| read, | ||
| remove, | ||
| write, | ||
| } |
There was a problem hiding this comment.
Added a redundancy in the file exports to ensure the compatibility of the import statements in existing JavaScript files using the default statement in a require statement.
There was a problem hiding this comment.
Let's remove this and just update all the references to this file!
This commit migrates the existing `src/utils/page-data.ts` file to TypeScript by trying to stick as close as possible to the original JavaScript file. To take advantage of existing Redux typings, I tried to extend the typings from the `IGatsbyPage` interface describing page data.
hiwelo
added
the
status: needs core review
| interface IPageData | ||
| extends Pick<IGatsbyPage, "componentChunkName" | "matchPath" | "path"> { | ||
| result: any | ||
| } |
There was a problem hiding this comment.
this is a bit of a nit. But I find these utility generics hard to understand. Can we change this to this?
interface IPageData {
componentChunkName: IGatsbyPage["componentChunkName"]
matchPath: IGatsbyPage["matchPath"]
path: IGatsbyPage["path"]
result: any // also, can this be improved?
} There was a problem hiding this comment.
I think any might be an instance of ExecutionResult from graphql. Try that out
|
|
||
| interface IPageDataContext { | ||
| publicDir: string | ||
| } |
There was a problem hiding this comment.
I know this isn't the scope of this, but can we actually change this to not wrap these in an object? All of the invocations that are like fn({ publicDir }) could and should just be fn(publicDir). So this will require updating the references too
|
|
||
| export const write = async ( | ||
| { publicDir }: IPageDataContext, | ||
| { componentChunkName, matchPath, path }: Exclude<IPageData, "result">, |
There was a problem hiding this comment.
I don't like this Exclude. We try to avoid complex typing. I think it would be easier to just have two interfaces. One that extends the other and adds result.
|
Closing because of missing fork branch, can't push an update. |
Description
This commit migrates the existing
src/utils/page-data.tsfile to TypeScript by trying to stick as close as possible to the original JavaScript file.To take advantage of existing Redux typings, I tried to extend the typings from the
IGatsbyPageinterface describing page data.Two typings are open to debate, the type returned by the promise line 24, and the typing for the
resultargument of thewritefunction.(also, first PR so I tried to stick to the contributing guidelines as much as possible, but let me know if something's off 😅)
Related Issues
Related to #21995