-
Notifications
You must be signed in to change notification settings - Fork 404
fix: avoid double encoding when notifying dev server of activity #7300
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
Conversation
📊 Benchmark resultsComparing with b4c6ca6
|
96cdf5d
to
b3b3f5e
Compare
@@ -365,7 +365,7 @@ const serveRedirect = async function ({ | |||
(await isEndpointExists(decodeURIComponent(reqUrl.pathname), options.target)) | |||
if (staticFile || endpointExists) { | |||
const pathname = staticFile || reqUrl.pathname | |||
req.url = encodeURI(pathname) + reqUrl.search | |||
req.url = encodeURI(decodeURI(pathname)) + reqUrl.search |
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.
why decodeURIComponent
on line 365 and decodeURI
here? can we use the same one and share the result?
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 iterate on this, I need this released faster, and this micro optimization not worth the additional testing effort
b3b3f5e
to
69d2a0d
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.
SGTM if you need this hotfixed and you've tested it manually, but afterward could you please add more context to the PR description and consider adding test coverage? 🙏🏼
Request: Once we merge this, can you come back through and add comments that explain the problem and why we're doing what we're doing in this code? And a test, if possible? The problem is non-obvious from this PR and from the code (I still don't know what the problem this solves is). |
Oh. Well...what Phil said. 😆 |
I've added it in the task btw, but I'm planning to put some refactor there, so I will add the comments into the code as well |
…oxying
🎉 Thanks for submitting a pull request! 🎉
Summary
Fixes double encoding and marking preview server activity.
For us to review and ship your PR efficiently, please perform the following steps:
passes our tests.
A picture of a cute animal (not mandatory, but encouraged)