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

Fixed decompression for responses without Content-Length header #5306

Merged
merged 5 commits into from Dec 1, 2022

Conversation

DigitalBrainJS
Copy link
Collaborator

@DigitalBrainJS DigitalBrainJS commented Nov 24, 2022

  • doesn't rely on content-length header, avoiding Z_BUF_ERROR;
  • added compress encoding to Accept-Encoding request header;
  • disabled decompression attempt for HEAD request;
  • add Brotli encoding to the Accept-Encoding header only if it is supported

Closes #5298
Closes #5296
Closes #5313;
Closes #5314;
Closes #5315;

Suggested release: 1.2.1

Copy link

@CarstenLeue CarstenLeue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The line

const responseLength = +res.headers['content-length'];

will still result in NaN for the responseLength which is then used for the download progress.
Does it make sense to show progress if no length is available?

@DigitalBrainJS
Copy link
Collaborator Author

DigitalBrainJS commented Nov 25, 2022

will still result in NaN

That is previously working code, it handles NaN.

Does it make sense to show progress if no length is available?

Of course, the length may not be present when the ProgressEvent emits.

@karlhorky
Copy link
Contributor

Does this also close #5311 ?

@DigitalBrainJS
Copy link
Collaborator Author

@karlhorky Unlikely. This PR fixes the regression bug with response decompression in v1.2.0, but it looks like that issue is related to request failure.
In any case, to be sure, it should be tested on v1.2.0-alpha.1 which does not have this bug.

@jsiegenthaler
Copy link

jsiegenthaler commented Nov 27, 2022

Hi @DigitalBrainJS I can confirm that 1.2.0-alpha.1 fixes the observations I made of uncompressed data in #5314
My core issue in #5314 was caused by incorrect headers on my part. But thanks for fixing the decompression. I'll close #5314

@karlhorky
Copy link
Contributor

karlhorky commented Nov 27, 2022

Yeah axios@1.2.0-alpha.1 also fixed my problem from #5311:

If @jamesmortensen also confirms that axios@1.2.0-alpha.1 fixed his problem, maybe this PR does indeed resolve #5311 as well.

@DigitalBrainJS
Copy link
Collaborator Author

@jasonsaayman Can we have a patch release with this?

@jasonsaayman
Copy link
Member

Yeah sure, I will get to this one asap

@davidfilat
Copy link

Yeah sure, I will get to this one asap

Any time...

@jasonsaayman jasonsaayman merged commit 9041c7d into axios:v1.x Dec 1, 2022
@deepak786
Copy link

When we can expect the new release 1.2.1?

@jasonsaayman
Copy link
Member

I am working on that, might be a 1.3.0 in all honesty

@DigitalBrainJS
Copy link
Collaborator Author

@jasonsaayman IMHO hotfixes are best released separately from minor changes, as such releases have a higher priority. This is more reliable because we can fix one thing but break another, probably not covered by tests, and the whole release becomes useless. Therefore, if we have a broken minor release (with a critical regression bug), we should release a patch to fix it, and only then publish a minor release with other changes.

@mlippert
Copy link

mlippert commented Dec 3, 2022

So axios@1.2.0-alpha.1 fixes the issue I just ran into after upgrading to 1.2.0.

I did want to point out that semantic version number wise, this prerelease probably should have been 1.2.1-alpha.1.

because when comparing versions: 1.2.0-alpha.1 < 1.2.0 < 1.2.1-alpha.1 < 1.2.1

Not really a big deal right now testing this, but thought I'd mention. You can see if you do the following:

$ npm i axios@1.2.0-alpha.1
$ npm outdated
Package        Current  Wanted  Latest  Location            Depended by
axios    1.2.0-alpha.1   1.2.0   1.2.0  node_modules/axios  riffcollector

@DigitalBrainJS
Copy link
Collaborator Author

DigitalBrainJS commented Dec 3, 2022

So axios@1.2.0-alpha.1 fixes the issue I just ran into after upgrading to 1.2.0.

@mlippert No, it doesn't fix the issue. It's just the last working version before v1.2.0 without the regression bug. Alpha was released 23 days ago. This fix has not yet been released.

@mlippert
Copy link

mlippert commented Dec 3, 2022

So axios@1.2.0-alpha.1 fixes the issue I just ran into after upgrading to 1.2.0.

@mlippert No, it doesn't fix the issue. It's just the last working version before v1.2.0 without the regression bug. Alpha was released 23 days ago. This fix has not yet been released.

Sorry, didn't realize that's what that release was. I just tried it and the failure I was seeing didn't occur, but that was the regression. I thought that release was a proposed fix. So that was the same as my re-installing 1.1.3 and gave you no useful information. I apologize, I got here via #5328.

@lucaswerkmeister lucaswerkmeister mentioned this pull request Dec 4, 2022
@MidnightTinge
Copy link

Sorry for the noise but any updates on a patch release? This is a non-obvious, app-breaking bug that wastes a lot of time tracking down. The only fix is to either disable decompression or downgrade, so it's a bit awkward that it's taking this long to get a patch out. I know people are busy, but I don't really think this is a back-burner kind of bug 😅

@DigitalBrainJS
Copy link
Collaborator Author

@MidnightTinge Unfortunately, this doesn't depend on me since I'm not a repo/npm collaborator. The only hope is that @jasonsaayman can find some time to make a release. For now, you can use the previous alpha version, as it is almost the same as this one, but working :)
Maybe we should make a CI release script to reduce the release effort and thus enable more frequent releases, but this task is also very labor-intensive.

@philBrown
Copy link

might be a 1.3.0 in all honesty

This is a terrible approach to semantic versioning that seems prevalent since v1.0.0. You seem to be continuously leaving a slew of broken minor releases in your wake. Anyone using ~version is left with absolutely no recourse.

Please do your users a favour and do patch releases.

@jasonsaayman
Copy link
Member

I will release it tonight, I will also release it as 1.2.1. I will also try to get to these quicker in the future. @philBrown thanks for that observation, there have been multiple issues fixed in those releases. Also whenever there is a type change I prefer to bump the minor otherwise there will be a lynch mob of people coming at me with threats or hate speech. It has been a rocky start to v1.x but I believe we are moving things in the right direction. I will try please as many people with versioning as I can but I can assure someone will hate what I do either way.

@jasonsaayman
Copy link
Member

Maybe we should make a CI release script to reduce the release effort and thus enable more frequent releases, but this task is also very labour-intensive.

@DigitalBrainJS I have been thinking about trying to get this done, I will probably look into it over the next while. I can also gran you a token to help with more, lets chat.

@jasonsaayman
Copy link
Member

@DigitalBrainJS I have elevated your access on this repo 😃

@jsiegenthaler
Copy link

@jasonsaayman I would like to express my thanks and gratitude to all the work you do on this project. As a maintainer of a few projects myself, I know just how much time and energy it can consume. And many project maintainers are dong it of their own free will, without any payment whatsoever. I will definitely not be a member of or driving any lynch mob . Once again, thanks for the great work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
10 participants