freeCodeCamp / classroom

BSD 3-Clause "New" or "Revised" License
144 stars 121 forks source link

fix: react hydration errors on admin #398

Closed Komal914 closed 1 year ago

Komal914 commented 1 year ago

Checklist:

Closes #361 Closes #360 Closes #359

This solution turns off server side rendering for the AdminTable component. I do not fully understand the benefits and tradeoffs of turning off server side rendering as I am still researching this topic. Let me know if you prefer server side to be on rather than off for AdminTable.

lloydchang commented 1 year ago

From CodeDay Labs Slack:

Komal Kaur 5:08 PM I was reading this: https://nextjs.org/docs/messages/react-hydration-error and tried solution 2

Lloyd Chang 6:44 PM @Komal Kaur While you are reading some more… Since you already have a fix via "solution 2," you can start preparing a pull request to solicit feedback publicly about "solution 2," and note in the pull request that you are reading about SSR in case a different solution that retains SSR is preferred by the maintainers.

lloydchang commented 1 year ago

https://github.com/freeCodeCamp/classroom/pull/398 lgtm as a good enough solution because Admin view isn't intended for Search Engine Optimization (SEO). I don't know why the errors were happening in the first place, but it may not matter if Server Side Rendering (SSR) can be disabled for the Admin view.

lloydchang commented 1 year ago

(edited) ~@Komal914 I believe this is no longer needed because #361, #360, #359 errors no longer happens after #401~

401 is merged and I'm still seeing this, so the fix isn't #401

GuillermoFloresV commented 1 year ago

Hi, @Komal914, thanks for taking the initiative on this issue.

What is the reasoning behind using Promise.resolve(<AdminTable ... ><AdminTable/>? Could we not import the adminTable component using next/dynamic like so: File: pages/admin/index.js

// The rest of your code inside of /admin/index.js
export default function Home() {

  const AdminTable = dynamic (() => import('path/to/adminTable'), ssr: false,});
  // The rest of your code inside of /admin/index.js 

  return {
  // The rest of your return code inside of /admin/index.js
  <AdminTable />
  // The rest of your return code inside of /admin/index.js
  }
}

?

Regardless, another change I think that we should do is to deprecate our use of react-data-table-component inside of the admin page and instead opt for react-table. You can find examples of how it is used locally to the project inside of dashtable v2.

The hope is that we do not find this issue inside of our new table library since the one the admin table is currently using has caused lots of issues for us before. If it does, however, we can attempt this fix that you have.

Komal914 commented 1 year ago

I did not realize we can simply import using dynamic with next.js. I will try the import instead and see if this takes care of the errors. Additionally I can update toreact-table as requested.

Komal914 commented 1 year ago

Hey @GuillermoFloresV please take a look at this PR again. I updated the import to use dynamic with no ssr for admintable. As for the table update, I think we should have that a separate issue as it requires me to research and dig into the data. I would be happy to work on it so feel free to assign me as I am experimenting with it already.