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

topic_images api: stop double-encoding params list #2237

Merged
merged 1 commit into from
Feb 9, 2024

Conversation

danielbeardsley
Copy link
Member

We added this in 8552518 along with several other correct url encoding changes.

Unfortunately, params.toString() already creates a correctly encoded string and encoding it again creates a doubly-encoded garbage url. That means this api hasn't worked at all since Nov-2023; it's been giving 404s.

This was just noticed now because one of our playwright tests loaded a product list page that made this request and resulted in a 404 which failed the test.

We added this in 8552518 along with several other correct url encoding
changes.

Unfortunately, params.toString() already creates a correctly encoded
string and encoding it again creates a doubly-encoded garbage url. That
means this api hasn't worked at all since Nov-2023; it's been giving
404s.

This was just noticed now because one of our playwright tests loaded a
product list page that made this request and resulted in a 404 which
failed the test.
Copy link

vercel bot commented Feb 9, 2024

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

Name Status Preview Updated (UTC)
react-commerce ✅ Ready (Inspect) Visit Preview Feb 9, 2024 9:27am
react-commerce-prod ✅ Ready (Inspect) Visit Preview Feb 9, 2024 9:27am

@ghost
Copy link

ghost commented Feb 9, 2024

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

Copy link
Contributor

github-actions bot commented Feb 9, 2024

📦 Next.js Bundle Analysis for @ifixit/commerce-frontend

This analysis was generated by the Next.js Bundle Analysis action. 🤖

This PR introduced no changes to the JavaScript bundle! 🙌

Copy link
Member

@danielcliu-ifixit danielcliu-ifixit left a comment

Choose a reason for hiding this comment

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

CR ✌️

@danielbeardsley
Copy link
Member Author

QA 👍

This branch:
image

Live:
image

Copy link
Contributor

@jarstelfox jarstelfox left a comment

Choose a reason for hiding this comment

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

CR 🌵

@danielbeardsley danielbeardsley merged commit acf4bb1 into main Feb 9, 2024
14 checks passed
@danielbeardsley danielbeardsley deleted the topic-images-api-url-encoding-fix branch February 9, 2024 17:30
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.

3 participants