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.

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

feat: V1 #7

wants to merge 8 commits into from

Conversation

ramyareye
Copy link
Collaborator

@ramyareye ramyareye commented Nov 27, 2022

Implementations:

  • Read and Sync with Google Sheet including provinces, cities and people entities
  • Generate local JSONs

closes #2

Copy link

@y-nk y-nk left a 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.

import type { Person, City, Province } from 'types';

const getPeople = () => {
return people;
Copy link

Choose a reason for hiding this comment

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

Suggested change
return people;
return people as Person[];

cast here instead of getData so that the types are also available granularly

};

const getProvinces = () => {
return provinces;
Copy link

Choose a reason for hiding this comment

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

Suggested change
return provinces;
return provinces as Province[];

const fileUrl = (type: DataTypes) =>
process.cwd() + `/public/data/${type}.json`;

export const readFile = async (type: DataTypes) => {
Copy link

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);
Copy link

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 = () => {
Copy link

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!${
Copy link

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 =
Copy link

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');
Copy link

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';
Copy link

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();
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Why don't we put effort on this?
2 participants