3.0 Release?

[MattMorgis]

Since #3009 was merged and a major version bump is required, I wanted to start a thread to discuss what else (if anything) should be included.

I see a 3.0 branch from years ago, but I'm unsure if there is any active work currently happening.

I'm sure there are some 🐛s that need squashed, but the backlog of issues and PRs is pretty overwhelming at the moment. I think @reconbot setting up statebot should help prioritize a bit.

[MattMorgis]

My personal preferences and ideas:

• first class support for native promises and async/await
• move docs async/await into main README.md
• deprecate bluebird & any promise implementations?

[mikeal]

first class support for native promises and async/await

The primary issue here is that request returns streams for nearly every interface and only buffers response bodies when asked to (given a callback). In order to implement this feature you'd need to:

• Return a Stream that is also a valid promise.
• Dynamically enable content buffering when .resolve() is called.
  • If .resolve() is called after data has already been streamed you'll need to error.

[mikeal]

Since #3009 was merged and a major version bump is required

Nope! Not true :)

Semver doesn't cover changes in the surrounding platform requirements, it is strictly related to the API exposed by the library.

We've already dropped support several times for Node.js versions that are past their long term support phase in minor releases of request.

[mikeal]

BTW, if promises were implemented the way I noted above you could ship them in a minor release as well, it makes it no longer a breaking change.

[reconbot]

I think it's kind to do a major release when dropping platform support. In any case I think there are a number of bug reports and prs that reference changing default behaviors, which would be a 3.0. I'll be able to get specific after stalebot does it's work and I have some more time. (this weekend) A Version is nothing but a number.

[mikeal]

The problem with doing a major release of request is that you're leaving out ~90% of the users. The vast majority of request users are not going to update to a new major and many of those libraries end up as deep dependencies in people's dep trees.

Leaving out 90% of the users in order to support a very small number that are using a now unsupported and insecure version of Node.js isn't right. Especially given than security fixes pretty much only land in the mainline of request, so you're sacrificing the security of 90% of the users that did the right thing and are running a secure version of Node.js. All of this so that a few people who are running an insecure version of Node.js can continue to run an eventually insecure version of request without seeing a failure they can quickly fix by locking the version (which they should have done in the first place if they are running such an old version of Node.js).

Essentially, you're introducing a silent security issue into the majority of the install base so that a couple people won't complain on twitter :)

[reconbot]

I'm assuming with that you also don't want to make any changes that wont run on node 4? So then we'd not actually dropping support? Just not testing on it?

[MattMorgis]

That is a very interesting approach to making promises a first class citizen.

Return a Stream that is also a valid promise.

Can you elaborate more on that? I understand the second part: if the consumer says .then() you buffer up the content fully instead of streaming. But I'm not grasping the mental model of a stream that is also a valid promise.

When triaging issues and PRs, I stumbled across some old discussions. This exposed the stream as a promise so you could await it and then .pipe() it somewhere or use a new async iterator on it, etc.

https://gist.github.com/MattMorgis/d3f44cd76dddbb20320ad8765f475251#file-js-L30

[mikeal]

@reconbot there's no reason to go out of our way to break Node.js v4.

But I'm not grasping the mental model of a stream that is also a valid promise.

First off, I said "resolve" but I meant "then" :)

The Promise API and the Stream API don't conflict in terms of naming. Any object with .then() and .catch() is a valid promise (last time I checked).

This exposed the stream as a promise so you could await it and then .pipe() it somewhere or use a new async iterator on it, etc.

If you await it you'll get the buffer data result, so you can't .pipe() the result and you won't be able to pipe the stream after you await it because all the data has finished :)

[mikeal]

I'll try to show a bit with some code.

let result = await request.get(url, {json:true})
result // {some: {object: "decoded from JSON"}}
let stream = request.get(url)
stream.pipe(dest)
let buffer = await stream
buffer // binary response body

[mikeal]

Oh! I just remembered another important part, the Stream/Promise returned result will still need to have the properties on it request uses to do object detection in pipes so that you can still do the on-line-proxy case :) This is already in the current implementation and there are tests for it, but just noting another thing we'll need to make sure is there :)

[MattMorgis]

Oooooh okay, I thought you were thinking along those lines — adding .then().

The gist I posted was me thinking in much simpler terms. It basically allows you to get the stream without using a callback. .on("response" just resolves with res. So you await and then can pipe or iterate over it. But yeah, just leaves you with the buffer to iterate over and I wasn't thinking of resolve and buffer up your data completely, so {json: true} wouldn't work, I don't think.

But you can do something like this, which I just confirmed worked with it:

  const stream = await requestPromiseStream(
    "https://assets-cdn.github.com/images/modules/logos_page/Octocat.png"
  );

  const dest = fs.createWriteStream("./octocat.png");
  stream.pipe(dest);

[MattMorgis]

I wonder if the approach in the gist can be adapted to match yours. Add a .then() that pauses the stream in a similar way and resolves with it.

Then if something like {json: true} is set, it instead collects all of the data and runs JSON.parse()

Definitely enough to start poking around

[mikeal]

So, the way it currently works is that if there is a callback it'll call readResponseBody https://github.com/request/request/blob/master/request.js#L1090

If you implement the Promise in such a way that calling .then() will simply add a callback to the instance you'll get the correct behavior.

[reconbot]

I think we got a lot of bugfixes before 3.0 but we're doing a disservice not dropping node 4 (and taking advantage of node 6's features) in a major update. I think I can get the tests passing on 4 again, if we want people to use the new features without making them upgrade, lets make sure they can use them. I do look forward to going 3.0+ and giving people the upgrade path.

[mikeal]

The primary thing you should probably look at for a 3.0 release is dropping some of the features that nobody uses and pushing them into some kind of plugin layer.

This has been attempted a few times but it didn't work out, but if you're doing a major breaking release that's the best time to drop several under-utilized features.

[reconbot]

Alright, HAR support doesn't work on node 4. Can't install the package much less use it.

This feels like a breaking change. But it's already broken, so.. idk whatevs

[reconbot]

Not going to have a 3.0 release see #3142 for more information.