sindresorhus / got

🌐 Human-friendly and powerful HTTP request library for Node.js
MIT License
14.3k stars 935 forks source link

Got 12 planning #1466

Closed sindresorhus closed 2 years ago

sindresorhus commented 4 years ago

There are a few issues: https://github.com/sindresorhus/got/milestone/9

Anything else we want to change that is risky or breaking? It doesn't have to be a big breaking release though.

// @szmarczak @Giotino

sindresorhus commented 4 years ago

@szmarczak I believe there was a breaking change we wanted to do to improve Node.js 15 compatibility?

szmarczak commented 4 years ago

Yes. It would introduce a new error event: retryError.

szmarczak commented 4 years ago

Should there be a separate branch for Got 12 development?

sindresorhus commented 4 years ago

I think https://github.com/sindresorhus/got/issues/1353 would also be breaking.

sindresorhus commented 4 years ago

Should there be a separate branch for Got 12 development?

No, we can get out one more minor release and then switch the focus of master to v12.

Giotino commented 4 years ago

What about this? https://github.com/sindresorhus/got/issues/1306#issuecomment-639687180

By not sending Got options directly to the request module we can have more control on the request, and we can swap the underlying request module without breaking Got (e.g. testing Undici and use it if it suits our needs).

This does not have to be breaking. I think we can achieve this without changing the options and the behavior.

sindresorhus commented 4 years ago

What about this? #1306 (comment)

👍🏻 Yes, we can do that one too.

szmarczak commented 4 years ago

Unless there's anything else I think we can release 11.7.0.

sindresorhus commented 4 years ago

Unless there's anything else I think we can release 11.7.0.

Done: https://github.com/sindresorhus/got/releases/tag/v11.7.0

We can start preparing for Got 12 now.

Giotino commented 4 years ago

Should there be a separate branch for Got 12 development?

I think we should use a new branch before the release, in order to continue patching the current release, if needed.

sindresorhus commented 4 years ago

I'd rather do a v11 branch for that, if we need to backport.

kanongil commented 4 years ago

There is note that http2 will be enabled by default for v12. Is this still the case, given the poor state of http2 support in node.js?

szmarczak commented 4 years ago

Is this still the case, given the poor state of http2 support in node.js?

I think we'll stick to HTTP/1.1. HTTP/2 is too much broken as of now and needs many improvements on the Node.js side, that's why the http2-wrapper development has stopped for a while.

sffc commented 3 years ago

Thanks for maintaining this project!

Just as general feedback, the frequent major version bumps on Got are disruptive and make it hard to use. I'm currently juggling at least 3 versions of Got throughout my projects, and I am trying to migrate them all to version 11, but pretty soon they will all be out of date again. The differences between the versions are mostly small changes to option names (like "query" versus "searchParams", the semantics of the "json" option, etc). TypeScript helps this somewhat, but I thought I'd advocate for putting more of a priority on API stability and making minor version releases instead of major ones.

P.S. I support @sindresorhus on Patreon.

sindresorhus commented 3 years ago

@sffc Got 10 was a pretty large breaking release. Out of necessity. Got had amassed a lot of legacy and bloat and we felt we had to clean it up, otherwise, it would be too hard to maintain Got going forward. The JSON change was the most important change as the previous JSON behavior was both surprising to new users, limiting and buggy. I'm very glad we did that change. The querysearchParams change was probably not necessary, but it does help new users familiar with Fetch in the browser. Going forward we don't plan on doing a lot of breaking changes, but we do need to do some once in a while. Our release notes are always detailed with migration steps for every breaking change.

As you can see from the previous releases, we have done approximately one major release a year, which I think it's ok and what many larger projects do:

Just know that we do care deeply about API stability.

sffc commented 3 years ago

Thanks! I have versions 6, 8, and 11 that I was reconciling. Glad to hear that the project is maturing with better stability moving forward.

bompus commented 3 years ago

Are there any plans to use undici for Got 12? I see it mentioned, but not sure what is involved with swapping in a different http request module.

sindresorhus commented 3 years ago

@bompus Not for Got 12, but we are closely following Undici development and might use it in the future.

sindresorhus commented 3 years ago

Just FYI. We're targeting Got v12 for the end of March.

Nytelife26 commented 3 years ago

I would suggest a bit of clean up with this release if possible. Ultimately, Got is way bigger than it needs to be, and also lacking in the performance department. Compared with a simple low level HTTP library that can do almost everything Got can,

Ultimately that is what has stopped me, all Kludge CS projects, Syrus, and many others from using Got. Just a bit of constructive criticism.

szmarczak commented 3 years ago

@Nytelife26 Most of the install size is TypeScript Node types. Consider opening an issue in the TypeScript repo.

sindresorhus commented 3 years ago

and also lacking in the performance department.

Any specifics or numbers to share? 🙏

Compared with a simple low-level HTTP library that can do almost everything Got can,

That is a common fallacy. From experience, the "almost" part eventually turns out to be much more. I'm talking from experience. Got actually started out as a 50 line package, but after years of being used in production, the messy reality of networking and countless of Node.js bugs has made Got larger than 50 lines.

szmarczak commented 3 years ago

I just noticed the more retries there are, the more errors will be created on promise.cancel():

https://github.com/sindresorhus/got/blob/4cce4de19ed577f481ba45a023b395a29cadce86/source/as-promise/index.ts#L43

The solution is to move it before makeRequest and use global request instead.

szmarczak commented 3 years ago

https://github.com/sindresorhus/got/blob/4cce4de19ed577f481ba45a023b395a29cadce86/source/as-promise/index.ts#L33 should reuse the previous request options on retry because promise.json() modifies the options

Nytelife26 commented 3 years ago

Any specifics or numbers to share?

Yes! I'm back with numbers, which you can see here, forked from your original Got benchmark suite (apologies for the delay).

off-topic > the "almost" part eventually turns out to be much more. Name something you can do with Got and I'm certain I can do it with the other faster libraries too, even if it requires a little more effort (but then again such things tend to be out of scope at the library level anyway). > Got actually started out as a 50 line package That's actually pretty impressive, I never would've guessed. A lot of your packages tend to be quite negatively impactful due to their oversharing amongst the ecosystem, however, you are definitely a competent developer in some respects.
szmarczak commented 3 years ago

Name something you can do with Got and I'm certain I can do it with the other faster libraries too, even if it requires a little more effort (but then again such things tend to be out of scope at the library level anyway).

https://github.com/sindresorhus/got#comparison

szmarczak commented 3 years ago

A lot of your packages tend to be quite negatively impactful due to their oversharing amongst the ecosystem, however, you are definitely a competent developer in some respects.

That "oversharing" turns out to be things needed by other developers.

Yes! I'm back with numbers, which you can see here, forked from your original Got benchmark suite (apologies for the delay).

Note that your package is lacking many features that others (such as request) have.

szmarczak commented 3 years ago

Note that in the future (a quite long time) ky can replace Got. https://github.com/nodejs/undici may provide a fetch interface in the near future.

szmarczak commented 3 years ago

Got 12 is going to be as fast as request (Stream API). Getting ~4800 +/- 7% req/s as of now.

szmarczak commented 3 years ago

The maximum performance (Stream API) Got 12 might reach is ~5500 req/s. In order to get more we would have to dump Duplex streams, which is not going to happen, as removing them adds a lot more complexity for the end user. I'm trying to make the Stream API look like Node.js native should had been at the beginning (but Duplex streams didn't exist at that point - nor they switched to them later). Attaching a response handler that returns a new stream is quite annoying.

Nytelife26 commented 3 years ago

https://github.com/sindresorhus/got#comparison

Sure, point me to the comparison page, but the comparison page doesn't include the fork, and I'm still certain I could do a lot of those things with the other packages, even if not out of the box.

That "oversharing" turns out to be things needed by other developers.

I am 110% certain a lot of things like read-pkg and pkg-up are not at all needed given their simplicity. As far as design philosophy goes, "a little copying is better than a little dependency". And then the larger packages, such as this one, are far out of scope for what they need to be and add way too much complexity. Thank the powers that be for tree shaking, right?

Note that your package is lacking many features that others (such as request) have.

The only thing that can't be done with our package at the moment to my knowledge is streaming. Care to provide examples?

Got 12 is going to be as fast as request (Stream API)

And using the normal API? Got is currently the slowest HTTP package out of all the primary ones on the ecosystem. I don't think adding that the stream API will be faster is of much use, since, largely the normal API needs work too. image

szmarczak commented 3 years ago

Sure, point me to the comparison page, but the comparison page doesn't include the fork,

I think you can figure out the differences on your own.

I could do a lot of those things with the other packages, even if not out of the box.

Ok, cool.

I am 110% certain a lot of things like read-pkg and pkg-up are not at all needed given their simplicity.

All Sindre's packages are unique - I don't consider them useless because you can do them on your own. If you're good with reinventing the wheel, no problem.

The only thing that can't be done with our package at the moment to my knowledge is streaming. Care to provide examples?

Yes, I've already given them to you. See the comparison table. But since you're lazy I'll give one right now: hooks.

And using the normal API?

I haven't done benchmarks w/ promises as I'm still working on them. I'll let you know when I do.

Got is currently the slowest HTTP package out of all the primary ones on the ecosystem.

If you deeply care about performance then you should go for undici. Mainly it's low-level but provides a high-level API as well.

I don't think adding that the stream API will be faster is of much use, since, largely the normal API needs work too.

People have different needs. We're trying our best.

Nytelife26 commented 3 years ago

I think you can figure out the differences on your own.

I can indeed, I'm just saying pointing me to the comparison table is useless when it doesn't include what we're talking about.

Ok, cool.

"Ok, cool" is hardly a valid response to concerns or constructive criticism.

All Sindre's packages are unique - I don't consider them useless because you can do them on your own. If you're good with reinventing the wheel, no problem.

This is partially NPM's fault, but it would be less harmful to the ecosystem to share them as copyable snippets. NPM installs every different version every package uses. So, while they're not useless, the harm outweighs the good in such cases.

Yes, I've already given them to you. See the comparison table. But since you're lazy I'll give one right now: hooks.

I'm not lazy, and once again, the comparison table is irrelevant when it doesn't include what we're talking about. I'm also picking up a fair amount of passive-aggressiveness, which isn't entirely appreciated.

I haven't done benchmarks w/ promises as I'm still working on them. I'll let you know when I do.

It has already been benchmarked for you using the current version, I was asking about improvements that'll come with the new version.

If you deeply care about performance then you should go for undici. Mainly it's low-level but provides a high-level API as well.

It was noble of you to suggest something else, admittedly, but that doesn't fix Got. Given its popularity, you should seriously consider improving performance, for the good of everyone using it.

People have different needs. We're trying our best.

I get that. Just make sure you don't focus all your effort on the stream API, which a lot of people will not use, when the normal API is the one that needs the most attention and improvement.

szmarczak commented 3 years ago
off-topic > when it doesn't include what we're talking about. I thought we were talking about features, weren't we? > NPM installs every different version every package uses. Every package manager does this. Forcing upgrades is breaking. > while they're not useless, the harm outweighs the good in such cases. I disagree here, but I'm not going to continue about this. > the comparison table is irrelevant when it doesn't include what we're talking about. "it doesn't include what we're talking about" doesn't make your point clear as well. I have directly mentioned the word "feature" and kept on linking to the comparison table which shows Got features. > I'm also picking up a fair amount of passive-aggressiveness, which isn't entirely appreciated. I'm not arguing, nor I'm going to. You have your own opinion - and that's right. I don't have to agree nor disagree. > "Ok, cool" is hardly a valid response to concerns or constructive criticism. I don't think > I could do a lot of those things with the other packages, even if not out of the box. is constructive criticism. You just expressed what you are capable of and it isn't about Got. So here's the same reply but more comprehensive: I acknowledge that you are capable of providing the same features Got has. I'm not saying you must not. I'm not saying Got is better. I'm not saying your solution is wrong. Feel free to use what you are comfortable with.

I'm going to mark our comments as off-topic now. If you wish to continue this discussion / start a new one, don't hesitate to open a new issue.

szmarczak commented 3 years ago

It has already been benchmarked for you using the current version

No, most of Got 12 is being worked on in a seperate PR.

It was noble of you to suggest something else,

Why would we be afraid of our competition? I disagree it was noble, it was just human. We have no interest in showing any package in a bad light. That would have a negative impact on the ecosystem.

you should seriously consider improving performance

You missed my comment, so here's a link: https://github.com/sindresorhus/got/issues/1466#issuecomment-803232739 and another one: https://github.com/sindresorhus/got/issues/1466#issuecomment-803214277

for the good of everyone using it.

Feel free to contribute! We've been developing Got for years - we know very well the direction Got is headed to. If anybody wants to replace Got with something else, it's okay! Use what you're comfortable with.

szmarczak commented 3 years ago

Released first beta: https://github.com/sindresorhus/got/releases/tag/v12.0.0-beta.1

danm commented 3 years ago

Hi!, any idea when v12 will land - is beta.4 safe to use? Thanks! ❤️

szmarczak commented 3 years ago

is beta.4 safe to use?

Yes, it's safe to use. It's been running in production for a long long time. We will do a release as soon as possible.

sindresorhus commented 2 years ago

Closing as we'll be releasing v12 soon.

szmarczak commented 2 years ago

Released got@12.0.0 :tada:

jfoclpf commented 1 year ago

My app started breaking in v12. Did you stop supporting require?

Error [ERR_REQUIRE_ESM]: require() of ES Module /home/joao/dev/geoapi.pt/node_modules/got/dist/source/index.js from /home/joao/dev/geoapi.pt/src/server/services/getNominatimData.js not supported.
Shinigami92 commented 1 year ago

My app started breaking in v12. Did you stop supporting require?

Error [ERR_REQUIRE_ESM]: require() of ES Module /home/joao/dev/geoapi.pt/node_modules/got/dist/source/index.js from /home/joao/dev/geoapi.pt/src/server/services/getNominatimData.js not supported.

Dude... NodeJS v14 reached EOL on 2023-04-30 https://github.com/nodejs/Release Please upgrade to NodeJS v16 immediately!

jfoclpf commented 1 year ago

@Shinigami92 I'm using node 16, thus I don't get your comment

$ node -v
v16.20.0

and NodeJs has no plans for deprecating CommonsJS https://github.com/nodejs/node/issues/33954

Shinigami92 commented 1 year ago

@Shinigami92 I'm using node 16, thus I don't get your comment


$ node -v

v16.20.0

Oh don't mind me, sorry. I thought you referring to NodeJS v12, my bad

jfoclpf commented 1 year ago

I meant that the app started breaking at got v 12. Downgraded to got v 11 and it worked again. Why remove support for CJS?

modestfake commented 1 year ago

@jfoclpf you can read the reasoning here https://github.com/sindresorhus/got/releases/tag/v12.0.0