search

hgouveia / node-downloader-helper

A simple http file downloader for node.js
MIT License
253 stars 54 forks source link

Fix content disposition parsing #76

Closed Chaphasilor closed 2 years ago

Chaphasilor commented 2 years ago

this closes #75

if you want me to change anything, just let me know!

Chaphasilor commented 2 years ago

hold on, this builds against Node 6 ‽
Maybe it's time to update the build target @hgouveia, the legacy url module is discontinued. It even was deprecated for some time...
I can fix the error for now, but we should really think about this :)

Chaphasilor commented 2 years ago

You're absolutely right, I think what happened is that the line endings got converted...

Chaphasilor commented 2 years ago

@paul-jarrow better now? :D

hgouveia commented 2 years ago

@Chaphasilor feel free to update the package to at least node 14 , and remove the legacyURL, node 6 was a requirement for an old project i was working on that uses this library and it wasnt possible to upgrade, but now we finally moved on to a newer backend

Chaphasilor commented 2 years ago

@Chaphasilor feel free to update the package to at least node 14 , and remove the legacyURL

@hgouveia okay, I'll do that in a different PR and try to clean up some of the older and deprecated things. maybe we should release it as a new major version (2.0.0) to make users aware of the breaking changes, but we can talk about it some more once I get around to the PR :)

hgouveia commented 2 years ago

@Chaphasilor agree lets up the version as suggested

Chaphasilor commented 2 years ago

@paul-jarrow I think if you don't see any other problems, @hgouveia could merge this now. maybe try to install the package from my branch via npm (npm i git+https://github.com/Chaphasilor/node-downloader-helper.git#fix-content-disposition-parsing) and do some real-world testing, just to be sure 😁

hgouveia commented 2 years ago

I will do some checking tomorrow before merge since I am not in the pc at the moment, but if @paul-jarrow could do some checking as well, if all fine I might also publish the new version tomorrow too since I think with all the changes from @Chaphasilor already deserve a version hehe

paul-jarrow commented 2 years ago

I will do some checking tomorrow before merge since I am not in the pc at the moment, but if @paul-jarrow could do some checking as well, if all fine I might also publish the new version tomorrow too since I think with all the changes from @Chaphasilor already deserve a version hehe

I think that I've done the checking I need to, thus the acceptance. It seems to work for the cases I can think of!