refactor(core): update with-routes to handle redirect logic better #1459

chanceaclark · 2024-10-14T15:50:42Z

What/Why?

Merging 3rd party code from #1429 (PR was created to run CI)

Testing

See CI

vercel · 2024-10-14T15:50:47Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
catalyst	🔄 Building (Inspect)	Visit Preview	💬 Add feedback	Oct 15, 2024 2:45pm
catalyst-latest	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 15, 2024 2:45pm

5 Skipped Deployments

Name	Status	Preview	Updated (UTC)
catalyst-1millionproducts-store	⬜️ Ignored (Inspect)	Visit Preview	Oct 15, 2024 2:45pm
catalyst-au	⬜️ Ignored (Inspect)	Visit Preview	Oct 15, 2024 2:45pm
catalyst-test-store	⬜️ Ignored (Inspect)	Visit Preview	Oct 15, 2024 2:45pm
catalyst-uk	⬜️ Ignored (Inspect)	Visit Preview	Oct 15, 2024 2:45pm
catalyst-unstable	⬜️ Ignored (Inspect)	Visit Preview	Oct 15, 2024 2:45pm

changeset-bot · 2024-10-14T15:50:47Z

🦋 Changeset detected

Latest commit: b2da4e0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@bigcommerce/catalyst-core	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

chanceaclark · 2024-10-14T17:26:09Z

core/middlewares/with-routes.ts

+        case 'BlogPostRedirect':
+        case 'BrandRedirect':
+        case 'CategoryRedirect':
+        case 'PageRedirect':
+        case 'ProductRedirect': {
+          // For dynamic redirects, assume an internal redirect and construct the URL from the path
+          const redirectUrl = new URL(route.redirect.to.path, request.url);
+
+          return NextResponse.redirect(redirectUrl, redirectConfig);


Hey @willPrattUPL I found a bug in the previous logic as it was rewriting, not redirecting. Can you test y'all redirects from this branch, specifically that 404 that had node return null?

I manually tested this with your 404 response, but added a path node to the "response". It seemed to work on my end but had to push a change to make sure trailing slashes were preserved correctly.

Hey, sorry Chance. We are extremely busy right now but I promise I'll get to this this week @chanceaclark

No worries, if it's good with the team I am going to merge. Feel free to test off main when you can and let us know if it's still an issue.

Stale

Co-authored-by: Chancellor Clark <[email protected]>

github-actions · 2024-10-15T22:07:28Z

⚡️🏠 Lighthouse report

Lighthouse ran against https://catalyst-latest-e4mnlcnrd-bigcommerce-platform.vercel.app

🖥️ Desktop

We ran Lighthouse against the changes on a desktop and produced this report. Here's the summary:

Category	Score
🟠 Performance	85
🟢 Accessibility	100
🟢 Best practices	96
🟠 SEO	82

📱 Mobile

We ran Lighthouse against the changes on a mobile and produced this report. Here's the summary:

Category	Score
🟢 Performance	93
🟢 Accessibility	100
🟢 Best practices	96
🟠 SEO	85

willPrattUPL · 2024-10-18T13:07:53Z

@chanceaclark

I've run this twice, both on the latest changes of my forked repo and our >2months old fork that we have been building off of

In both cases I get this:

on all redirects

#1429 (comment)

/uplift-v2-standing-desk-v2-or-v2-commercial/
/uplift-v2-l-shaped-standing-desk/
/uplift-v2-standing-desk-frame/
/height-adjustable-standing-desk-with-l-shaped-top/

All redirects exhibit the exact same behavior as before, except now the 404 is an infinite redirect loop

[
@bigcommerce/catalyst-core:dev: {
@bigcommerce/catalyst-core:dev: "route": {
@bigcommerce/catalyst-core:dev: "redirect": {
@bigcommerce/catalyst-core:dev: "to": {
@bigcommerce/catalyst-core:dev: "__typename": "ProductRedirect",
@bigcommerce/catalyst-core:dev: "path": "/uplift-v2-l-shaped-standing-desk/"
@bigcommerce/catalyst-core:dev: },
@bigcommerce/catalyst-core:dev: "toUrl": "https://www.upliftdesk.com/uplift-v2-l-shaped-standing-desk/"
@bigcommerce/catalyst-core:dev: },
@bigcommerce/catalyst-core:dev: "node": {
@bigcommerce/catalyst-core:dev: "__typename": "Product",
@bigcommerce/catalyst-core:dev: "id": "UHJvZHVjdDo4NTk=",
@bigcommerce/catalyst-core:dev: "entityId": 859
@bigcommerce/catalyst-core:dev: }
@bigcommerce/catalyst-core:dev: },
@bigcommerce/catalyst-core:dev: "expiryTime": 1729257562395
@bigcommerce/catalyst-core:dev: },
@bigcommerce/catalyst-core:dev: {
@bigcommerce/catalyst-core:dev: "status": "LAUNCHED",
@bigcommerce/catalyst-core:dev: "expiryTime": 1729256851939
@bigcommerce/catalyst-core:dev: }
@bigcommerce/catalyst-core:dev: ]

Frustratingly, this actually is lending a lot of credence to the theory that the actual root cause is route.node.id ending in "="

Please see link to previous PR comments with my redirect console logging to confirm, but each one ends in "=" ( #1429 (comment) )

willPrattUPL · 2024-10-18T13:34:40Z

There might be something in the API creation where if route.node.id ends in "=" it is erroneously adding both a node and a redirect <

^This is all likely not correct. Keeping for historical posterity

chanceaclark · 2024-10-18T14:40:47Z

Looked into this and seems there were some 301 redirects configured to redirect back onto itself e.g. /some-url -> /some-url/. They are going to look into cleaning those up and will let us know if it's still broken.

chanceaclark requested a review from a team as a code owner October 14, 2024 15:50

chanceaclark force-pushed the chore/3rd-party branch from 7f73656 to 964b073 Compare October 14, 2024 15:50

vercel bot deployed to Preview – catalyst-latest October 14, 2024 15:54 View deployment

deini previously approved these changes Oct 14, 2024

View reviewed changes

chanceaclark added the do not merge label Oct 14, 2024

chanceaclark marked this pull request as draft October 14, 2024 16:24

chanceaclark force-pushed the chore/3rd-party branch 2 times, most recently from 1b466b5 to ffdf652 Compare October 14, 2024 16:28

chanceaclark marked this pull request as ready for review October 14, 2024 16:31

vercel bot deployed to Preview – catalyst-latest October 14, 2024 16:33 View deployment

chanceaclark force-pushed the chore/3rd-party branch from ffdf652 to c77abca Compare October 14, 2024 17:24

chanceaclark commented Oct 14, 2024

View reviewed changes

vercel bot deployed to Preview – catalyst-latest October 14, 2024 17:26 View deployment

chanceaclark force-pushed the chore/3rd-party branch 2 times, most recently from 3702888 to 407f83b Compare October 15, 2024 14:29

chanceaclark requested a review from a team October 15, 2024 14:29

vercel bot deployed to Preview – catalyst-latest October 15, 2024 14:33 View deployment

chanceaclark force-pushed the chore/3rd-party branch 2 times, most recently from 33e0773 to 979157b Compare October 15, 2024 14:40

chanceaclark changed the title ~~Chore/3rd party~~ refactor(core): update with-routes to handle redirect logic better Oct 15, 2024

vercel bot deployed to Preview – catalyst-latest October 15, 2024 14:42 View deployment

refactor(core): update with-routes to handle redirect logic better

b2da4e0

Co-authored-by: Chancellor Clark <[email protected]>

chanceaclark force-pushed the chore/3rd-party branch from 979157b to b2da4e0 Compare October 15, 2024 14:43

vercel bot deployed to Preview – catalyst-latest October 15, 2024 14:45 View deployment

chanceaclark requested a review from deini October 15, 2024 15:28

chanceaclark removed the do not merge label Oct 15, 2024

deini approved these changes Oct 15, 2024

View reviewed changes

chanceaclark added this pull request to the merge queue Oct 15, 2024

Merged via the queue into main with commit b4485c7 Oct 15, 2024
13 checks passed

chanceaclark deleted the chore/3rd-party branch October 15, 2024 22:03

github-actions bot mentioned this pull request Oct 15, 2024

Version Packages #1457

Merged

chanceaclark mentioned this pull request Oct 16, 2024

Product redirects when Base64 id ends in "=" never resolve their redirect (Broken page) #1320

Closed

This was referenced Aug 7, 2024

Version Packages bookernath/catalyst#1

Open

Version Packages willPrattUPL/catalyst#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(core): update with-routes to handle redirect logic better #1459

refactor(core): update with-routes to handle redirect logic better #1459

chanceaclark commented Oct 14, 2024

vercel bot commented Oct 14, 2024 •

edited

Loading

changeset-bot bot commented Oct 14, 2024 •

edited

Loading

chanceaclark Oct 14, 2024

chanceaclark Oct 15, 2024

willPrattUPL Oct 15, 2024

chanceaclark Oct 15, 2024

github-actions bot commented Oct 15, 2024

willPrattUPL commented Oct 18, 2024 •

edited

Loading

willPrattUPL commented Oct 18, 2024 •

edited

Loading

chanceaclark commented Oct 18, 2024

refactor(core): update with-routes to handle redirect logic better #1459

refactor(core): update with-routes to handle redirect logic better #1459

Conversation

chanceaclark commented Oct 14, 2024

What/Why?

Testing

vercel bot commented Oct 14, 2024 • edited Loading

changeset-bot bot commented Oct 14, 2024 • edited Loading

🦋 Changeset detected

chanceaclark Oct 14, 2024

Choose a reason for hiding this comment

chanceaclark Oct 15, 2024

Choose a reason for hiding this comment

willPrattUPL Oct 15, 2024

Choose a reason for hiding this comment

chanceaclark Oct 15, 2024

Choose a reason for hiding this comment

github-actions bot commented Oct 15, 2024

⚡️🏠 Lighthouse report

🖥️ Desktop

📱 Mobile

willPrattUPL commented Oct 18, 2024 • edited Loading

willPrattUPL commented Oct 18, 2024 • edited Loading

chanceaclark commented Oct 18, 2024

vercel bot commented Oct 14, 2024 •

edited

Loading

changeset-bot bot commented Oct 14, 2024 •

edited

Loading

willPrattUPL commented Oct 18, 2024 •

edited

Loading

willPrattUPL commented Oct 18, 2024 •

edited

Loading