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

Implement pause option for resolving with stream #16

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mildsunrise
Copy link

See request/request-promise#90. The particular implementation details are:

  • transform and resolveWithFullResponse are always ignored.
  • The stream is not paused if the promise rejects (i.e. if simple = true and the response is a 404), so the response cannot be streamed in that case. This is a limitation, but 99% of the time users aren't interested in the error body, so that'd be a memory leak unless they make sure stream.resume() is called on rejections. And if you really need to stream error responses, you can use simple = false.

Proposed documentation and examples for the option:

var options = {
    uri: 'http://my-server/big-file.zip',
    pause: true,
};
rp(options)
    .then(function (response) {
        console.log("Downloading %s bytes file...", response.headers["content-length"]);
        response.pipe(fs.createWriteStream("downloaded.zip"));
        response.on("end", () => console.log("Downloaded."));
    })
    .catch(function (err) {
        // Request failed...
    });
  • pause = false which is a boolean that when set, the promise will be resolved with the raw response stream (http.IncomingMessage) before the body arrives (e.g. when the response event fires). Implies resolveWithFullResponse = true and transform = null.
    • Note: The stream will be paused before the promise resolves, so that you can pipe or subscribe to the stream before any data gets lost. .pipe() and others resume it automatically, otherwise you may need to call .resume() manually, even if you do nothing else with the stream.
    • Note 2: You don't need to do anything if the promise rejects, as the stream is not paused. But this means it's unsafe to stream the response in that case, as data might get lost.

The implementation is in the second commit, the first is just a refactor to prepare for the changes.

- Initialize options at the beginning of init
- Extract the code that resolves / rejects the promise into separate functions
@coveralls
Copy link

Coverage Status

Coverage decreased (-3.4%) to 96.61% when pulling dab29c7 on botgram:feature-pause into 7c4f307 on request:master.

5 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-3.4%) to 96.61% when pulling dab29c7 on botgram:feature-pause into 7c4f307 on request:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-3.4%) to 96.61% when pulling dab29c7 on botgram:feature-pause into 7c4f307 on request:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-3.4%) to 96.61% when pulling dab29c7 on botgram:feature-pause into 7c4f307 on request:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-3.4%) to 96.61% when pulling dab29c7 on botgram:feature-pause into 7c4f307 on request:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-3.4%) to 96.61% when pulling dab29c7 on botgram:feature-pause into 7c4f307 on request:master.

@mildsunrise
Copy link
Author

Does this look good to you? If it does, I'll add tests

@forivall
Copy link

Thanks for finally submitting a PR - I never got around to it since i had my custom resolve-stream-from-promise wrapper around request and never needed it anywhere else, and I'm not using request anywhere in my current projects.

The changes I can see being needed are documentation, and as this option means that the stream is being resolved, personally, i'd call the option pausedStream or something of that nature.

Also, on errors, in my old proof of concept, (and what I did in the previously-mentioned wrapper) is to read the body on error, rather than completely discarding it. IMO, in simple mode, it would be better to read the body on error as the default behaviour, and add (yet another) option to discard the body on error in simple mode.

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