-
Notifications
You must be signed in to change notification settings - Fork 276
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]: Improve performance for redirects #SXA-7834 #2003
base: dev
Are you sure you want to change the base?
[sitecore-jss-nextjs]: Improve performance for redirects #SXA-7834 #2003
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks as a good improvement! I added some comments
Of course you need to add unit tests, they are missing :)
We can jump on the call to discuss some uncertainties
packages/sitecore-jss-nextjs/src/middleware/redirects-middleware.ts
Outdated
Show resolved
Hide resolved
packages/sitecore-jss-nextjs/src/middleware/redirects-middleware.ts
Outdated
Show resolved
Hide resolved
packages/sitecore-jss-nextjs/src/middleware/redirects-middleware.ts
Outdated
Show resolved
Hide resolved
packages/sitecore-jss-nextjs/src/middleware/redirects-middleware.ts
Outdated
Show resolved
Hide resolved
…ded to improve the performance of redirects. Additionally, a condition has been added to handle Netlify requests to reduce server load.
f67d456
to
9879878
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I see that you removed caching mechanism, do we still need it for standard requests? Or it's enough to skip prefetch requests to get a stable performance?
- Unit tests are missing (e.g. new prefetch req case)
} | ||
|
||
// Skip prefetch requests | ||
if (this.isPrefetch(req)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add some explanation on the reason of skipping prefetch requests, for better understanding if other people will need to figure out the scenario
locale, | ||
}); | ||
} else { | ||
matchedQueryString = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me looks like this bunch of code does almost the same what getPermutedQueryMatch
does
do you just need to use the same method?
pattern: redirect.pattern, | ||
locale, | ||
}); | ||
if (req.headers.get('cdn-loop') === NAME_NETLIFY) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a better way to safely check for netlify? Env variables?
Another question - do we really need to do this check, since :261 line does almost the same what getPermutedQueryMatch
does. Does it really optimize something? since the same reg exp is executed
|
||
const REGEXP_CONTEXT_SITE_LANG = new RegExp(/\$siteLang/, 'i'); | ||
const REGEXP_ABSOLUTE_URL = new RegExp('^(?:[a-z]+:)?//', 'i'); | ||
const NAME_NETLIFY = 'netlify'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case you leave netlify check (see my question below)
const NAME_NETLIFY = 'netlify'; | |
const NETLIFY_CDN_HEADER = 'netlify'; |
@@ -339,7 +363,7 @@ export class RedirectsMiddleware extends MiddlewareBase { | |||
* @param {string} [params.locale] - The locale prefix to include in the URL if present. | |||
* @returns {string | undefined} - return query string if any of the query permutations match the provided pattern, undefined otherwise. | |||
*/ | |||
private isPermutedQueryMatch({ | |||
private getPermutedQueryMatch({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method, which generates all permutations of query parameters to test against regex patterns, can lead to performance degradation due to its factorial time complexity (O(n!)). Can this method/approach be refactored to avoid the performance degradation that leads to timeouts in all hosting envs (including netlify and vercel). This would address #2004
Description / Motivation
A condition for prefetch requests has been added to improve the performance of redirects.
Additionally, a condition has been added to handle Netlify requests to reduce server load.
Testing Details
Types of changes