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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG 馃悳] Fix Server Rendering on /account Page #1675

Merged
merged 12 commits into from
Feb 26, 2024

Conversation

bendvc
Copy link
Collaborator

@bendvc bendvc commented Feb 20, 2024

Description

There was a regression when doing accessibility work in the following PR. What is happening is that when you load the /account page on the server we are actually rendering the page contents, and with the previous change we were using the document object which threw an error.

I did 2 things in this PR...

  1. Don't render the /account page when on the server.
  2. Replace the dom document usage with react useRef.

Types of Changes

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Documentation update
  • Breaking change (could cause existing functionality to not work as expected)
  • Other changes (non-breaking changes that does not fit any of the above)

Changes

  • Render nothing for account page on the server. We don't want to redirect because it would render the login page even if the user is logged in on the client. Making it a janky experience.
  • Swap document access for useRef.

How to Test-Drive This PR

> git checkout bug/fix-account-server-render
> npm ci
> cd packages/template-retail-react-app
> npm start

Scenario 1: Unauthenticated user accesses the account page.

  1. Open browser and goto http://localhost:3000/account
  2. Notice that there isn't a server error
  3. You should be brought to the login page via a client-side redirect.

Scenario 2: Authenticated user accesses the account page.

  1. Open browser and goto http://localhost:3000
  2. Login
  3. You will be taken to the account page.
  4. Refresh your browser.
  5. No error is thrown and you see your account information after is loads client-side.

Checklists

General

  • Changes are covered by test cases
  • CHANGELOG.md updated with a short description of changes (not required for documentation updates)

Accessibility Compliance

You must check off all items in one of the follow two lists:

  • There are no changes to UI

or...

Localization

  • Changes include a UI text update in the Retail React App (which requires translation)

Copy link
Collaborator

@joeluong-sfcc joeluong-sfcc left a comment

Choose a reason for hiding this comment

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

Thanks for taking a look and fixing this @bendvc. Confirmed with manual testing that we no longer see the server error when going to http://localhost:3000/account. However, I'm seeing this other error in the browser console:

Screenshot 2024-02-21 at 12 18 11鈥疨M

Wonder if the easiest way around this is to use forwardRef()?

@bendvc
Copy link
Collaborator Author

bendvc commented Feb 21, 2024

Thanks for taking a look and fixing this @bendvc. Confirmed with manual testing that we no longer see the server error when going to http://localhost:3000/account. However, I'm seeing this other error in the browser console:

Screenshot 2024-02-21 at 12 18 11鈥疨M

Wonder if the easiest way around this is to use forwardRef()?

Thanks for spotting that. I have have committed a fix for this issue.

joeluong-sfcc
joeluong-sfcc previously approved these changes Feb 21, 2024
@bendvc bendvc marked this pull request as ready for review February 21, 2024 21:50
@bendvc bendvc requested a review from a team as a code owner February 21, 2024 21:50
bendvc and others added 2 commits February 22, 2024 08:29
Signed-off-by: Ben Chypak <bchypak@mobify.com>
@@ -107,14 +107,19 @@ const Account = () => {
navigate('/login')
}

// Do not render anything in the account page on the server!
if (!onClient) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we no longer use the document object, it should be ok to server side render the account page right? Asking because it seems that we're seeing lots of hydration errors

Screenshot 2024-02-22 at 5 04 30鈥疨M

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can't remember if we were always rendering the account page on the server or if it was added by mistake, but by nature all the data that it displays is private customer data and shouldn't be rendered on the client.

So this is one of the isomorphic development problems that you run into here and there. Because you are always a guest on the server you would be redirected to the login page which would be rendered and sent to the client. Upon loading the app on the client, it would see that you are registered and send you to the account page. So you would get this login page > account page flow.

If you leave the code you commented on in, then you simply have nothing rendered, then the account page. Those errors that you see are only ever shown in develop mode and not in production.

But I guess it all depends on what the behaviour was before the introduction of that breaking change.

Do you happen to know what that was?

Copy link
Collaborator

Choose a reason for hiding this comment

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

From my understanding, before the breaking change we would render the static account page on the server and then hydrate once it got to the client. I don't see any logic that prevents server side rendering in the previous version: https://github.com/SalesforceCommerceCloud/pwa-kit/blob/0843abc9c9dd05d3d9f3a4a2aab211dae4940aa9/packages/template-retail-react-app/app/pages/account/index.jsx

Seeing the login page then account page if you're a registered user doesn't seem like good UX, nothing rendered then account page is probably better. If these errors are only shown development and not production, then I think we should leave in that logic to prevent rendering on the server

bendvc and others added 2 commits February 22, 2024 16:02
Co-authored-by: Joel Uong <88680517+joeluong-sfcc@users.noreply.github.com>
Signed-off-by: Ben Chypak <bchypak@mobify.com>
@bendvc bendvc merged commit d01360e into develop Feb 26, 2024
28 checks passed
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.

None yet

3 participants