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

Move to use async functions #197

Merged
merged 3 commits into from Sep 18, 2017
Merged

Move to use async functions #197

merged 3 commits into from Sep 18, 2017

Conversation

@hueniverse
Copy link
Member

hueniverse commented Sep 17, 2017

No description provided.

@hueniverse hueniverse added this to the 12.5.1 milestone Sep 17, 2017
README.md Outdated
const req = wreck.request(method, uri, options, optionalCallback);
const promise = wreck.request(method, uri, options);
// Use promise.req.abort() to terminate the request early

This comment has been minimized.

Copy link
@kanongil

kanongil Sep 17, 2017

Member

This is not that simple. promise.req will be the wrong one, if the request has been redirected.

This comment has been minimized.

Copy link
@hueniverse

hueniverse Sep 17, 2017

Author Member

How is this different from previous code? The ability to abort the request was mostly there for testing. It might not need to survive this rewrite.

This comment has been minimized.

Copy link
@kanongil

kanongil Sep 17, 2017

Member

Aborting is useful for some use-cases. Anyway, it's primarily the comment I have an issue with, as it implies that aborting will just work like that.

This comment has been minimized.

Copy link
@hueniverse

hueniverse Sep 17, 2017

Author Member

Fair.

}
catch (err) {
expect(err.output.statusCode).to.equal(504);
}

This comment has been minimized.

Copy link
@kanongil

kanongil Sep 17, 2017

Member

This is not a safe way to test. The expect() won't be called if the request is successful, causing it to unexpectedly pass.

We probably need something like hapijs/code#106, for a simple safe solution.

@geek geek self-requested a review Sep 17, 2017
Copy link
Member

geek left a comment

Very minor typos... thank you for the updates

README.md Outdated
});
};
Use `promise.req.abort()` to terminate the request early. Note that this is limited to the initial request only.
If the was request already redirected, aborting the original request will not abort execution of pending redirections.

This comment has been minimized.

Copy link
@geek

geek Sep 17, 2017

Member

missing a word:

If the was

README.md Outdated
If a callback function is provided then an instance of the node.js [ClientRequest](http://nodejs.org/api/http.html#http_class_http_clientrequest) object is returned.
If no callback function is provided then a Promise is returned that resolves an object with the following structure: `{ req, res, payload }`;

Thows any error that may have occurred during handling of the request or a Boom error object if the response has an error status

This comment has been minimized.

Copy link
@geek

geek Sep 17, 2017

Member

typo:

Thows

README.md Outdated
object, which is a readable stream that has "ended" and contains no more data to read.
- `payload` - The payload in the form of a Buffer or (optionally) parsed JavaScript object (JSON).

If a callback function is provided then an instance of the node.js [ClientRequest](http://nodejs.org/api/http.html#http_class_http_clientrequest) object is returned.
If no callback function is provided then a Promise is returned that resolves an object with the following structure: `{ req, res, payload }`;
Thows any error that may have occurred during handling of the request or a Boom error object if the response has an error status

This comment has been minimized.

Copy link
@geek

geek Sep 17, 2017

Member

typo:

Thows

README.md Outdated
If a callback function is provided then an instance of the node.js [ClientRequest](http://nodejs.org/api/http.html#http_class_http_clientrequest) object is returned.
If no callback function is provided then a Promise is returned that resolves an object with the following structure: `{ req, res, payload }`;

Thows any error that may have occurred during handling of the request or a Boom error object if the response has an error status

This comment has been minimized.

Copy link
@geek

geek Sep 17, 2017

Member

typo:

Thows

README.md Outdated
If a callback function is provided then an instance of the node.js [ClientRequest](http://nodejs.org/api/http.html#http_class_http_clientrequest) object is returned.
If no callback function is provided then a Promise is returned that resolves an object with the following structure: `{ req, res, payload }`;

Thows any error that may have occurred during handling of the request or a Boom error object if the response has an error status

This comment has been minimized.

Copy link
@geek

geek Sep 17, 2017

Member

typo:

Thows

README.md Outdated
If a callback function is provided then an instance of the node.js [ClientRequest](http://nodejs.org/api/http.html#http_class_http_clientrequest) object is returned.
If no callback function is provided then a Promise is returned that resolves an object with the following structure: `{ req, res, payload }`;

Thows any error that may have occurred during handling of the request or a Boom error object if the response has an error status

This comment has been minimized.

Copy link
@geek

geek Sep 17, 2017

Member

typo:

Thows

});

server.listen(0, () => {
it('requests a resource with callback', async () => {

This comment has been minimized.

Copy link
@geek

geek Sep 17, 2017

Member

Can update the test title: retrieves a resource

@hueniverse hueniverse merged commit d792f7b into master Sep 18, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@hueniverse hueniverse deleted the promises branch Sep 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.