-
Notifications
You must be signed in to change notification settings - Fork 385
semver: do not support upload() from url #337
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
Conversation
b3a6928
to
00efcd1
Compare
Codecov Report
@@ Coverage Diff @@
## master #337 +/- ##
=======================================
Coverage ? 100%
=======================================
Files ? 7
Lines ? 1016
Branches ? 0
=======================================
Hits ? 1016
Misses ? 0
Partials ? 0
Continue to review full report at Codecov.
|
Why not support it? |
Remove the upload() from url functionality. Instead, use File.createWriteStream. For performance reasons, nodejs-storage should not require the `request` module. As we are currently exposing `request` as part of the public API for `upload()`, not using request is a breaking change. Instead of using a different dependency for the http requests, we leave this decision to the users. If you were using `upload()` from a url, replace it with the following: const request = require('request'); let readStream = request(url).createReadStream(); const file = bucket.file(name) const writeStream = file.createWriteStream(); readStream.pipe(writeStream) Use `node-fetch` instead of `request` in system tests.
00efcd1
to
229733b
Compare
Sorry, didn't include the reasoning, see updated commit message. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Before merging I would like to take some time to make sure we are prepared to incr semver-major.
Heads up @Kiricon-- we're going to be removing the feature you added for us. Do you think it's a mistake? The dilemma is that it forces us to keep a dependency on We have a couple of options:
Thoughts and ideas welcome! |
It's been a week with no objections, can we merge this? |
I consider this as a mistake. There are no "perfomance reasons" to do this. It's now harder to upload files, without any benefits performance-wise. Welp. |
Remove the upload() from url functionality. Instead, use
File.createWriteStream.
For performance reasons, nodejs-storage should not require the
request
module. As we are currently exposing
request
as part of the publicAPI for
upload()
, not using request is a breaking change. Instead ofusing a different dependency for the http requests, we leave this decision
to the users.
If you were using
upload()
from a url, replace it with the following:Use
node-fetch
instead ofrequest
in system tests.