Skip to content

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

Merged
merged 4 commits into from
Aug 22, 2018
Merged

Conversation

fhinkel
Copy link
Contributor

@fhinkel fhinkel commented Aug 15, 2018

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.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 15, 2018
@ghost ghost assigned fhinkel Aug 15, 2018
@fhinkel fhinkel force-pushed the removeUploadFromUrl branch from b3a6928 to 00efcd1 Compare August 15, 2018 03:01
@codecov
Copy link

codecov bot commented Aug 15, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@e68242a). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##             master   #337   +/-   ##
=======================================
  Coverage          ?   100%           
=======================================
  Files             ?      7           
  Lines             ?   1016           
  Branches          ?      0           
=======================================
  Hits              ?   1016           
  Misses            ?      0           
  Partials          ?      0
Impacted Files Coverage Δ
src/bucket.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e68242a...cfaa816. Read the comment docs.

@stephenplusplus
Copy link
Contributor

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.
@fhinkel fhinkel force-pushed the removeUploadFromUrl branch from 00efcd1 to 229733b Compare August 15, 2018 14:29
@fhinkel
Copy link
Contributor Author

fhinkel commented Aug 15, 2018

Sorry, didn't include the reasoning, see updated commit message.

@fhinkel fhinkel requested a review from crwilcox August 15, 2018 14:31
Copy link
Contributor

@crwilcox crwilcox left a 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.

@crwilcox crwilcox added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Aug 15, 2018
@stephenplusplus
Copy link
Contributor

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 request, which we are trying to avoid (it's big, and it has introduced some security concerns).

We have a couple of options:

  1. Get rid of the feature. The user can handle fetching the data themselves, and pipe it into a file.createWriteStream(). This is definitely not as easy as "put this remote file in my bucket". The user will have to create a File object, and familiarize themselves with the streaming APIs that both our library and their possibly-new HTTP-fetching library.
  2. Use a different request library, but somehow remain library-agnostic when accepting common HTTP options, like headers.

Thoughts and ideas welcome!

@ghost ghost assigned jkwlui Aug 21, 2018
@fhinkel
Copy link
Contributor Author

fhinkel commented Aug 22, 2018

It's been a week with no objections, can we merge this?

@crwilcox crwilcox merged commit f520326 into master Aug 22, 2018
@stephenplusplus stephenplusplus deleted the removeUploadFromUrl branch September 10, 2018 17:46
@nnmrts
Copy link

nnmrts commented Sep 17, 2018

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.

@ncruces ncruces mentioned this pull request Oct 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement. do not merge Indicates a pull request not ready for merge, due to either quality or timing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants