cemgunay / coleaseum-webapp

COLEASEUM
https://coleaseum-webapp.vercel.app
1 stars 0 forks source link

mainly just added SSR functionality and Pusher real time implementation #8

Closed cemgunay closed 1 year ago

cemgunay commented 1 year ago

this commit fixes #4

Did the following things in this PR:

SSR

In pages/listing/[listingId].js

Pusher Context

API Routes

Added api/requests/create.js route

Added api/requests/update.js route

Pusher Subscribe and Bind

In pages/listing/[listingId].js

Components

index.js

utils/pusher.js

utils/connectMongo.js

Misc

nathanclairmonte commented 1 year ago

This is great!! Took a look at everything and it's perfect, also checked out the branch to run it on my end and everything seems to be working perfectly.

Here are few thoughts/potential changes I had, not saying we absolutely have to make these changes or anything like that, just some thoughts. Let me know what you think!

  1. I realised while reviewing that we're using diff indentation styles. Most of your files that you changed are set to indents of 2 spaces and mine are set to indents of 4 spaces. Noticed it bc my VSCode like resets everything to 4 spaces on save so when I was adding console.logs and stuff all the files would be reset to 4 spaces. Not a huge deal I don't think, like everything should still work together whether it's 2 or 4 spaces, but maybe we should streamline? Honestly not sure, I can look into like collab work with two diff indentation styles as well if we wanna keep our individual ones. But TLDR don't think it's a big deal rly
  2. For the /pages/listing/[listingId] page, we should probably memoize the computed state variables (at lines 56-70) to make sure they're only re-computed when the listing prop changes and not with every single re-render. Something like this:
// Derived state or computations
const { formattedAddress, numBeds, numBedrooms, numBathrooms, formattedRoomInfo, images } = useMemo(() => {
  if (!listing) {
    return {
      formattedAddress: "",
      numBeds: 0,
      numBedrooms: 0,
      numBathrooms: 0,
      formattedRoomInfo: "",
      images: [],
    };
  }

  const formattedAddress = `${listing.location.address1}, ${listing.location.city}, ${listing.location.stateprovince}`;
  const numBeds = listing.basics.bedrooms.map((bedroom) => bedroom.bedType).length;
  const numBedrooms = listing.basics.bedrooms.length;
  const numBathrooms = listing.basics.bathrooms;
  const formattedRoomInfo = `${numBeds} bed${numBeds === 1 ? "" : "s"} • ${numBedrooms} bedroom${numBedrooms === 1 ? "" : "s"} • ${numBathrooms} bathroom${numBathrooms === 1 ? "" : "s"}`;
  const images = listing.images.map(({ url }) => url);

  return { formattedAddress, numBeds, numBedrooms, numBathrooms, formattedRoomInfo, images };
}, [listing]);
  1. We should prob add an if (!listing) return; line at the top of the useEffect on line 79 of the same /pages/listing/[listingId] page. Just to safeguard against app crashes in the event of something going wrong with the DB or the SSR function. For e.g. before I had defined my NEXT_PUBLIC_API_URL in the env variables, no listing prop was coming through and we were getting crashes. Not sure how likely it is that the listing prop won't come through but idk I still think it's best to plan for like worst case just in case yk
  2. In a similar line of thinking (and on the same /pages/listing/[listingId] page too) I think we should still keep the original main <Loading/> component as well as the new ones, just in case there's some delay with the DB or the SSR function. Hopefully unlikely cuz it's SSR, but I think it's better to have a loading screen there just in case the listing prop takes a few seconds to come through. Here's the original main Loading component so u don't have to go lookin for it if u agree w this idea ahah:
// loading component
const Loading = () => {
    return (
        <div className="flex flex-col items-start justify-start h-screen gap-3">
            <Skeleton className="h-60 w-full rounded-none mb-6" />
            <Skeleton className="h-8 w-24 mx-4 mb-2" />
            <Skeleton className="h-6 w-52 mx-4 mb-8" />
            <Skeleton className="h-6 w-80 mx-4" />
            <Skeleton className="h-6 w-96 mx-4" />
            <Skeleton className="h-6 w-72 mx-4" />
            <Skeleton className="h-6 w-80 mx-4" />
            <Skeleton className="h-6 w-72 mx-4" />
        </div>
    );
};

// show loading page until listing is successfully retrieved
if (!listing) {
    return <Loading />;
}

Aaand I think that's it in terms of changes

In terms of the merge conflicts, I was looking at them and they're all relatively easy to resolve I think. So I'll hold off on resolving them for now, and then once you've looked at these thoughts and either vetoed them or made the changes I'll handle it then!

cemgunay commented 1 year ago

Sweeet thanks for catching all that Nathan. Here's what I did let me know what you think:

  1. I was using Prettify and it was set to 2 indent I changed it to 4 so that should be good now I hope let me know.
  2. Makes hella sense, I made the changes.
  3. And 4. For this, Since getServerSideProps runs at request time on the server and completes before the page is rendered, there's no client-side fetching or waiting period that would even show a loading page. The page rendered on the client will already have all the data that getServerSideProps fetched or an indication of an error. So I took what you said and learned that getServerSideProps has some error handling that I implemented and tested.
    • If API Url is buggy it goes to 404 page
    • If the fetch function fails to execute properly and returns an error it also goes to 404 page
    • If the fetch successfully executes but HTTP response is an error
    • If error status 404 go to 404 page
    • If any other error status go to 500 page
    • In the future I can add seperate redirects for different fetch errors
    • 401 unathorized
    • 403 forbidden
    • 400 bad request
    • etc

I also just quickly added 404 and 500 pages

Let me know your thoughts and if this makes sense!!

nathanclairmonte commented 1 year ago

ezzz yes it makes sense! everything looks great, gonna resolve the merge conflicts now

nathanclairmonte commented 1 year ago

okay sorry that took ages lmfao I ran into some huge issues with the merge conflicts, specifically the tailwind.config.js file (had some clashes with the shadcn-ui component library) and the package-lock.json file (merge conflicts in that file are so annoying lol). got everything sorted in the end tho, and ran and tested all the new functionality too to make sure the merge didn't introduce any new errors. just committed and pushed the conflict resolution changes and gonna merge into main now!

cemgunay commented 1 year ago

You're the goat 🐐