Skip to content
This repository was archived by the owner on Mar 3, 2026. It is now read-only.

semver: do not support upload() from url#337

Merged
crwilcox merged 4 commits into
masterfrom
removeUploadFromUrl
Aug 22, 2018
Merged

semver: do not support upload() from url#337
crwilcox merged 4 commits into
masterfrom
removeUploadFromUrl

Conversation

@fhinkel
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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 subscribe to this conversation on GitHub. Already have an account? Sign in.

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