Skip to content
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.

Sign up for GitHub

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(gatsby): Migrate utils/page-data to TypeScript #23436

Closed
wants to merge 1 commit into from
Closed

chore(gatsby): Migrate utils/page-data to TypeScript #23436

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 0 additions & 52 deletions packages/gatsby/src/utils/page-data.js
View file
Open in desktop

This file was deleted.

79 changes: 79 additions & 0 deletions packages/gatsby/src/utils/page-data.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
import fs from "fs-extra"
import path from "path"
import { IGatsbyPage } from "../redux/types"
import { store } from "../redux"

interface IPageDataContext {
publicDir: string
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


interface IPageData
extends Pick<IGatsbyPage, "componentChunkName" | "matchPath" | "path"> {
result: any
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think any might be an instance of ExecutionResult from graphql. Try that out


export const fixedPagePath = (pagePath: string): string =>
pagePath === `/` ? `index` : pagePath

const getFilePath = (
{ publicDir }: IPageDataContext,
pagePath: string
): string =>
path.join(publicDir, `page-data`, fixedPagePath(pagePath), `page-data.json`)

export const read = async (
{ publicDir }: IPageDataContext,
pagePath: string
): Promise<IPageData> => {
const filePath = getFilePath({ publicDir }, pagePath)
const rawPageData = await fs.readFile(filePath, `utf-8`)

return JSON.parse(rawPageData)
}

export const remove = async (
{ publicDir }: IPageDataContext,
pagePath: string
): Promise<void> => {
const filePath = getFilePath({ publicDir }, pagePath)

if (fs.existsSync(filePath)) {
return await fs.remove(filePath)
}

return Promise.resolve()
}

export const write = async (
{ publicDir }: IPageDataContext,
{ componentChunkName, matchPath, path }: Exclude<IPageData, "result">,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

result: IPageData["result"]
): Promise<void> => {
const filePath = getFilePath({ publicDir }, path)
const body = {
componentChunkName,
path,
matchPath,
result,
}
const bodyStr = JSON.stringify(body)
// transform asset size to kB (from bytes) to fit 64 bit to numbers
const pageDataSize = Buffer.byteLength(bodyStr) / 1000

store.dispatch({
type: `ADD_PAGE_DATA_STATS`,
payload: {
filePath,
size: pageDataSize,
},
})

await fs.outputFile(filePath, bodyStr)
}

export default {
fixedPagePath,
read,
remove,
write,
}
Comment on lines +74 to +79
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove this and just update all the references to this file!