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

[sitecore-jss-nextjs] Refactor RedirectsMiddleware for Better Extensibility #2040

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

ah100101
Copy link

@ah100101 ah100101 commented Feb 7, 2025

This PR refactors the RedirectsMiddleware class to improve its extensibility and maintainability by:

  1. Extracting the main redirect processing logic into a protected method
  2. Making getExistsRedirect protected to allow override in derived classes

Description / Motivation

Extending RedirectsMiddleware or MiddlewareBase currently requires re-implementing the entire handler. Changing getExistsRedirect to a protected method and encapsulating the handler will make it easier to extend RedirectsMiddleware and add optimizations like caching or logic to bypass fetches to Sitecore under certain criteria.

This PR makes the following changes:

  1. Creates new protected method processRedirectRequest that encapsulates the core redirect logic
  2. Changed getExistsRedirect access modifier from private to protected
  3. Simplified the handler to delegate to processRedirectRequest

Testing Details

Existing functionality in RedirectsMiddleware remains the same. These changes were tested in both development and production deployed applications on Vercel.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

@ah100101 ah100101 marked this pull request as ready for review February 7, 2025 22:21
@yavorsk
Copy link
Contributor

yavorsk commented Feb 11, 2025

Solid work, thanks for the contribution!
I added this in our backlog for review and prioritization.

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.

2 participants