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: Fixes a server client boundary error #3917

Merged
merged 6 commits into from
Jul 3, 2024
Merged

fix: Fixes a server client boundary error #3917

merged 6 commits into from
Jul 3, 2024

Conversation

thiessenp-cds
Copy link
Contributor

@thiessenp-cds thiessenp-cds commented Jun 28, 2024

Summary | Résumé

Fixes a prod error where NextJS tried to server render some client code. Here is the prod error for reference: "ReferenceError: sessionStorage is not defined"

Test

One way to test this is to programmatically trigger the MFA page to sever side render (See Bryan's comment below).
Do this by:

Reproduce the error

  1. Checkout the develop branch and add a button like the below on the login page (outside the form element)
      <Button onClick={() => { window.location.href = `/${language}/auth/mfa/resend`; }}>
        test
      </Button>
  1. Click the button and watch your dev console, you should see something like this
    on develop

Verify the error is gone

  1. Checkout fix/3915 (this branch) and add the button like in the above
  2. Click the button and the error in the dev console about sessionStorage being undefined should be gone

@thiessenp-cds thiessenp-cds linked an issue Jun 28, 2024 that may be closed by this pull request
Copy link
Contributor

@thiessenp-cds thiessenp-cds marked this pull request as ready for review June 28, 2024 15:00
const existingFlow = useRef<{ email?: string; authenticationFlowToken?: string; error?: string }>(
{}
);
useEffect(() => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I really wanted to use the new React hook use() but ran into some errors and an open bug. I wasn't able to find a work around so I sadly went with the old style.
For reference:
facebook/react#26930
and then this
vercel/next.js#51477

@thiessenp-cds thiessenp-cds changed the title fix/Fixes a server client boundary error fix: Fixes a server client boundary error Jun 28, 2024
@timarney
Copy link
Member

timarney commented Jul 3, 2024

@thiessenp-cds can you add test instructions ?

@thiessenp-cds
Copy link
Contributor Author

@thiessenp-cds can you add test instructions ?

This is a difficult one to test. The error only happens sometimes and manually throwing an error etc. won't really help since it's a hydration related. For this fix, I think all we can do is test that it doesn't break anything. (still works)
@bryan-robitaille can you think of any way to test this?

@bryan-robitaille
Copy link
Contributor

This is a difficult one to test. The error only happens sometimes and manually throwing an error etc. won't really help since it's a hydration related. For this fix, I think all we can do is test that it doesn't break anything. (still works) @bryan-robitaille can you think of any way to test this?

This error was probably happening because somewhere on our site we are still using a standard <a> tag that references this page or a redirect from the server. If you can find how users navigated to the page that triggered the server render instead of client side navigation you'll be able to test. Reference: https://stackoverflow.com/questions/57376992/in-nextjs-how-to-programatically-trigger-server-side-rendering

@timarney
Copy link
Member

timarney commented Jul 3, 2024

:)

Can just test by visiting https://localhost:3000/en/auth/mfa/resend direct

@thiessenp-cds thiessenp-cds merged commit 877d3af into develop Jul 3, 2024
13 checks passed
@thiessenp-cds thiessenp-cds deleted the fix/3915 branch July 3, 2024 16:36
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.

ReVerify error about sessionStorage not being defined
3 participants