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

Handle errors thrown synchronously by http.request(). #210

Closed
wants to merge 1 commit into from

Conversation

satazor
Copy link
Contributor

@satazor satazor commented Jul 11, 2016

const got = require('got');

got('htttp://google.com')
.then((res) => console.log('res', res), (err) => console.log('error', err));

The code above fails globally because htttp is not a valid protocol. This PR fixes it.

@floatdrop
Copy link
Contributor

I believe this code should catch such errors (and we have similar test).

Also this test is green:

test('invalid url', async t => {
    got('htttp://google.com').then(t.fail).catch(err => {
        t.regex(err.message, /Protocol .+ not supported/);
        t.pass();
    });
});

Can you make a PR with failing test?

@satazor
Copy link
Contributor Author

satazor commented Jul 11, 2016

@floatdrop sorry the test is wrong, I should have used got.stream since the bug is there.

How should we adjust this then?

@satazor
Copy link
Contributor Author

satazor commented Jul 11, 2016

@floatdrop maybe it's ok to throw synchronously since this method does not return a promise?

@floatdrop
Copy link
Contributor

maybe it's ok to throw synchronously since this method does not return a promise?

@satazor I think so.

@satazor
Copy link
Contributor Author

satazor commented Jul 11, 2016

Agree! Closing this.

@satazor satazor closed this Jul 11, 2016
@satazor satazor deleted the fix-invalid-url branch July 11, 2016 17:53
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

2 participants