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

[NextJS] Add prefetchLinks parameter to RichText component #1517

Conversation

pschofield
Copy link
Contributor

@pschofield pschofield commented Jun 7, 2023

Description / Motivation

When the RichText component contains a large number of internal links, the number of network calls to prefetch these links can be quite large. It would be ideal to have the ability to disable the prefetch step, similar to how the next/link component allows the prefetch to be enabled/disabled. The existing default behaviour (prefetch is performed) would be maintained.

Testing Details

  • Unit Test Added
  • Manual Test/Other (Please elaborate)
    Manual testing involved applying the proposed change to a local dev instance of Sitecore 10.3 and verifying that link prefetching didn't occur when prefetchLinks was set to false. Also observed that prefetching occurs when prefetchLinks is either not explicitly set or set to true.

Types of changes

Subsequent pages don't have client side routing

Added a useEffect dependancy to the NextJs RichText component. This
fixes an issue where where the useEffect() hook doesn't run when
navigating to a page that contains a RichText component after clicking an
internal link in a RichText component on another page to cause the
navigation.

Description:
1. Open a page that contains a RichText component that has an <a> tag
   that links to a page within the same site (i.e. clicking the link
   fires the routeHandler() function within the richText component that
   performs a client side navigation)

2. When the RichText component on the new page renders, it doesn't run
   the useEffect hook so the event handler doesn't get attached to any
   internal links within the RichText component

3. Clicking on any of the embedded links causes a whole page refresh as
   the event handler isn't attached, so no client side routing occurs.
Add 'prefetchLinks' parameter to NextJs RichText component to allow
users to turn prefetching of internal links on and off.

This can reduce the amount of data fetch, particularly on a page with
many links that are unlikely to be visited.
Copy link
Contributor

@sc-addypathania sc-addypathania left a comment

Choose a reason for hiding this comment

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

@pschofield Thankyou for your contribution.
I think this is a great idea and would allow users or developers to have more control over the trade-off between performance and resource usage.

Approved 👍🏼
Please update the changelog so I can merge the PR later :)

@sc-addypathania sc-addypathania changed the title [NextJS] New Feature - Add prefetchLinks parameter to RichText component [NextJS] Add prefetchLinks parameter to RichText component Jun 8, 2023
@pschofield
Copy link
Contributor Author

@pschofield Thankyou for your contribution. I think this is a great idea and would allow users or developers to have more control over the trade-off between performance and resource usage.

Approved 👍🏼 Please update the changelog so I can merge the PR later :)

Hi @sc-addypathania
thanks, I've updated the changelog

@pschofield
Copy link
Contributor Author

And fixed merge conflict

Copy link
Contributor

@illiakovalenko illiakovalenko left a comment

Choose a reason for hiding this comment

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

👍

@sc-addypathania sc-addypathania merged commit 9a2c6b2 into Sitecore:dev Jun 9, 2023
1 check passed
@pschofield pschofield deleted the feature/jss-nextjs-richtext-add-prefetch-parameter branch June 11, 2023 21:07
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