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

Fix location encoding #214

Merged
merged 3 commits into from
Aug 29, 2016
Merged

Fix location encoding #214

merged 3 commits into from
Aug 29, 2016

Conversation

luanmuniz
Copy link
Contributor

@sindresorhus
Copy link
Owner

Needs a test

@luanmuniz
Copy link
Contributor Author

On it

@sindresorhus
Copy link
Owner

// @jamestalmage FYI, for follow-redirects.

@luanmuniz
Copy link
Contributor Author

luanmuniz commented Aug 1, 2016

@sindresorhus Its fixed, but i'm having a small problem if compatibility.

I found 2 ways to fix this:
new Buffer(res.headers.location, 'binary').toString()
and
Buffer.from(res.headers.location, 'binary').toString()

While the first one is deprecated in v6 the second doesn't exist in v4 and since the tests check for 4, 5 and 6 i don't know how it should be.

Obs.: It's working with the first option
Obs2.: Don't merge it yet, i will write tests for this case

@luanmuniz
Copy link
Contributor Author

luanmuniz commented Aug 14, 2016

@sindresorhus Done. Ready to merge

Just one thing. I tried to add the following test:

http.on('/redirect-with-utf8', (req, res) => {
    res.writeHead(302, {
        location: `${http.url}/utf8-url-áé`
    });
    res.end();
});

test('redirect response contains utf8', async t => {
    t.is((await got(`${http.url}/redirect-with-utf8`)).body, 'reached');
});

But than the retry -> works on timeout errordon't pass.
Any idea what it can be?

@luanmuniz
Copy link
Contributor Author

@sindresorhus Anything else to do here?

@floatdrop
Copy link
Contributor

@luanmuniz lgtm, thanks ⭐

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.

None yet

3 participants