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

fix: react hydration errors on admin #398

Merged
merged 7 commits into from
Aug 17, 2023

Conversation

Komal914
Copy link
Contributor

@Komal914 Komal914 commented Jul 27, 2023

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.

@Komal914 Komal914 requested a review from a team as a code owner July 27, 2023 05:53
@lloydchang
Copy link
Contributor

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 lloydchang mentioned this pull request Jul 27, 2023
2 tasks
@lloydchang
Copy link
Contributor

lloydchang commented Jul 28, 2023

#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
Copy link
Contributor

lloydchang commented Jul 31, 2023

(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
Copy link
Member

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
Copy link
Contributor Author

Komal914 commented Aug 9, 2023

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 to react-table as requested.

@Komal914
Copy link
Contributor Author

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.

Copy link
Collaborator

@utsab utsab left a comment

Choose a reason for hiding this comment

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

Thank you @Komal914 and @theGaryLarson for fixing this hydration error. Turning off server side rendering seems like a reasonable approach.

@utsab utsab merged commit 9623d31 into freeCodeCamp:main Aug 17, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment