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.

Sign up for GitHub

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

Jump to bottom

v2: Better UTF-8 URL handling #1290

Closed
jlowcs opened this issue Sep 14, 2021 · 3 comments
Closed

v2: Better UTF-8 URL handling #1290

jlowcs opened this issue Sep 14, 2021 · 3 comments

Comments

@jlowcs
Copy link

jlowcs commented Sep 14, 2021

Is your feature request related to a problem? Please describe.

Since v3 is ESM only and your recommendation is to use v2 is ESM if this is blocking for us, could it be possible to port this improvement to v2?

Indeed, we already had a few instances of a SSR issue in production where the fetched url contained UTF-8 characters and made the fetch fail server-side but not client-side. Those issues are pretty tricky to catch ahead of time if the developer forgets to take UTF-8 into account when building the url.

Describe the solution you'd like

Port the UTF-8 improvement to v2.
@LinusU
Copy link
Member

LinusU commented Sep 14, 2021

  1. Do you have an example of an url that is failing that we could use as a test case?
  2. Would you be able to submit a PR for this?

@jlowcs
Copy link
Author

jlowcs commented Sep 14, 2021

  1. Here's a very simple test case:
require('node-fetch')('http://www.foo.com/ひらがな')

image

  1. Well, I would assume that you would have a pretty good idea on how to implement that fix since it was implemented in v3, while it will probably take me more time just because I'm not familiar with the codebase. That being said, if you give me the necessary pointers so I don't spend too much time on this, I suppose I could take care of that PR.

@LinusU LinusU mentioned this issue Sep 14, 2021
Merged
1 task
@LinusU
Copy link
Member

LinusU commented Sep 14, 2021

I've submitted a PR for this here: #1291

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@LinusU @jimmywarting @jlowcs