-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat: V1 #7
base: main
Are you sure you want to change the base?
feat: V1 #7
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.
left a bunch of comments as requested, but approving since i've no authority on this project.
congratulations for your involvement in this project.
src/data/index.ts
Outdated
import type { Person, City, Province } from 'types'; | ||
|
||
const getPeople = () => { | ||
return people; |
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.
return people; | |
return people as Person[]; |
cast here instead of getData so that the types are also available granularly
src/data/index.ts
Outdated
}; | ||
|
||
const getProvinces = () => { | ||
return provinces; |
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.
return provinces; | |
return provinces as Province[]; |
const fileUrl = (type: DataTypes) => | ||
process.cwd() + `/public/data/${type}.json`; | ||
|
||
export const readFile = async (type: DataTypes) => { |
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.
you can use fs/promises i guess
}; | ||
|
||
export const writeFile = (type: DataTypes, file: string) => { | ||
fs.writeFileSync(fileUrl(type), file); |
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.
doesn't hurt to promisify this too imho
cities = cities.map((ct) => (ct.id === city.id ? city : ct)); | ||
}; | ||
|
||
export const writeCities = () => { |
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.
if writeFile is promisified u can save a little time with a Promise.all
['F', 33], | ||
]; | ||
|
||
const sheetRange = `Provinces!${ |
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.
this sounds pretty magical lol. i would rather go for notion api, looks less fragile.
or you can leverage this instead https://www.npmjs.com/package/google-spreadsheet
as an optimization i suggest you to used the row metadata property to store the uuid of your record. it is invisible to google spreadsheet so people cannot tamper the data, and it's more optimized when you want to fetch one row by id.
{ responseType: 'stream' } | ||
); | ||
|
||
const fileName = |
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.
this i really dont get it 😂
import { get as getSheetData, update as updateSheetData } from 'utils/api'; | ||
|
||
const fetch = async (req: NextApiRequest, res: NextApiResponse) => { | ||
const data = await getSheetData('2022!D2:E300'); |
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.
if (req.method !== "get") res.status(404).end()
?
@@ -0,0 +1,130 @@ | |||
import type { NextApiRequest, NextApiResponse } from 'next'; |
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.
best pattern in next.js is to avoid coding business logic inside the api route directly, otherwise you will block yourself from doing SSR.
better to do business logic in a separate function and import it, such as (simplified):
export default async function (req, res) {
const data = zod.validate(req.query)
try {
const result = await myLibFn(data)
res.status(200).json(result)
} catch (err) {
res.status(500).json({ err })
}
}
@@ -14,7 +16,10 @@ export default function ListItem({ | |||
onClick: (ref: React.MutableRefObject<unknown>, person: Person) => void; | |||
}) { | |||
const clickRef = useRef(null); | |||
const { city, province } = getCityProvince(person.city); | |||
const { getCity, getProvince } = useData(); |
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.
imho best would be to leverage getServerSideProps instead of using a data provider so that you can enable SSR.
Implementations:
closes #2