[discussion] library choice changes -- requests, vinyl

[dignifiedquire]

As some might have seen or read I'm currently suggesting to move away from using raw http.request and if possible to drop the code in src/get-files-stream.js, which is mostly using vinyl to generate a nested multipart requests of the files to be uploaded to the API. But as @jbenet pointed out on IRC I have not properly explained why I make these suggestions and why I think doing so would be very beneficial for this module.

To understand my reasoning let me formulate the main goals for this project as I understand them:

1. Provide a small and simple wrapper around the http API of ipfs daemon
2. 100% compatibility between node and browser
3. Do not reinvent/reimplement the wheel, unless there is a very good reason to do so

Part 1: Drop direct usage of http.request

A lot of the code in this project is reimplementing things that already have been implemented and battle tested in other modules, when it comes to the usage of http.request. Two main projects come here to mind request and superagent. As explored in #92 the only request module that will do everything we need and has excellent support for node and browser seems to be request. The initial reason for exploring these options for me was #69 which has strong implications for the stability of station.

Part 2: Drop vinyl if possible

First vinyl on its own is a representation of the file system in memory and a nice API around it to access it. This taken on itself is nothing bad in any way, but does not need to be in this module in my opinion as it adds additional bloat which in the browser has very little relevance, as the power of vinyl comes from it being able to actually access the file system in the background to read it in.

we'll still need something like it (a js object to represent a file before reading it all in).
@jbenet (IRC)

From my understanding, we do not need a representation of a file object in this API wrapper. Application code around this API might very well need it, but for the purposes of uploading files via the http API there are more straight forward ways to do it:

1. [browser][node]: The content of a file and the filename are passed: We can directly generate a multipart request from this.
2. [node]: The path to a file is passed: Use fs.createReadStream to read the file from the file system and generate the multipart stream
3. [node]: The path of a directory is passed: Use something like glob to walk the directory and convert into the same structure as 2.
4. [browser]: The content of a directory of files is passed: Use the conversion from 3. to generate the multipart request.

I hope this explanation makes my thinking and efforts more understandable and sorry again for not writing this up earlier but parts of this were simply only understood by me when doing the research and work.

[jbenet]

Thanks for the thorough write up @dignifiedquire

notes for discussion:

on "we'll still need something like it (a js object to represent a file before reading it all in" i mean that we need some way (in node) of scanning the directory, building up a stream of files to add, and processing them one by one. important to not actually read the files until later (but maybe important to grab an fd, not sure how safe we want the semantics to be). vinyl-fs does this for us, by scanning a dir hierarchy first and creating streams but not yet reading them.

I do want to preserve the ability to input a node stream of files that the api adds. (we could easily write a helper for this, but it is a well used use case).

I do like the ability to support globs for the fs.

it is critical to support node streams natively.

I don't like vinylfs -- i dont like many things about it, including how it forces globs, or isnt super portable, it's big and complicated.

I think we need more input mechanisms:

1. [node/browser] A stream of data passed in directly from another source. This becomes one file, no need for a filename. this should be fine.
2. [node/browser] A stream of streams of hierarchical data is passed in (i.e. a stream of files, like vinyl-stream). i do this in a few modules already, and there's lots of utils that expose this interface.
3. [node/browser] A user passes in one of the many kinds of Buffer, or a string. this is just the contents of a file.

All of these become easier with an object that carries the filename and the contents, with the contents being a flexible value (i.e. buffers, string, stream, etc). This is effectively what vinyl is. If you dont want vinyl because it's large fine, but the abstraction is a good one and makes life much easier.

by the way, the multipart stream module is likely to stay-- need some way to turn a stream of files into a stream of multipart contents. That module (though confusing to read) works fine. it's not worth spending time reinventing the wheel there. only if it has problems should we address it.

Worth noting that vinyl is a wrapper to make the going from path -> stream of files part really easy and reliable across filesystems.

---

in my view, the vinyl support works today, and it does not at all force itself on the user. the user can pass a variety of objects in directly, and even a simple dict like {path: "filename.jpg", contents: fs.createReadStream("otherfilename.jpg") }.

i still don't understand well what we gain from expending work to remove it. The main concern i see above:

• additional bloat which in the browser has very little relevance

I do not like vinyl much, but it does work fine. If the size of the lib is the concern, that's a concrete issue, but then it really is about just raw data, and it better be well worth it.

othwerwise, i'm not yet convinced this is a large enough concern to spend the energy (a) changing this, (b) creating a new abstraction, and (c) changing all the dependent modules (and breaking the api on users we cannot see).

---

Otherwise, then let's first make the vinyl-like thing that's super light (maybe just {path: <path>, contents: <contents> } and does the (a) reading hierachies from disk, (b) makes stream of file streams, (c) does all the contents type handling (streams, buffers, strings, etc), (d) does the reverse -- writing back to disk equally well.

[dignifiedquire]

Thanks @jbenet for the extensive answer. After the considerations I think we can close this now, that we request is being used and seems to do a good job integrated with the full vinyl solution 😃

[jbenet]

@dignifiedquire ok sounds good.

(i dont mean to dissuade from questioning + refactoring, that stuff is often useful. sometimes the choices are there because of a lot of concerns that may not be apparent up front, and might mean a lot of time to invest to reach similar level, etc. it's always a tradeoff on time)

[Mithgol]

By the way, npmtree of dependencies of vinyl-fs-browser is quite large:

• brfs@^1.4.1
  • quote-stream@^1.0.1
    • buffer-equal@0.0.1
    • minimist@^1.1.3
    • through2@^2.0.0
      • readable-stream@~2.0.0
        • core-util-is@~1.0.0
        • inherits@~2.0.1
        • isarray@0.0.1
        • process-nextick-args@~1.0.0
        • string_decoder@~0.10.x
        • util-deprecate@~1.0.1
      • xtend@~4.0.0
  • resolve@^1.1.5
  • static-module@^1.1.0
    • concat-stream@~1.4.5
      • inherits@~2.0.1
      • typedarray@~0.0.5
      • readable-stream@~1.1.9
        • core-util-is@~1.0.0
        • isarray@0.0.1
        • string_decoder@~0.10.x
        • inherits@~2.0.1
    • duplexer2@~0.0.2
      • readable-stream@~1.1.9
        • core-util-is@~1.0.0
        • isarray@0.0.1
        • string_decoder@~0.10.x
        • inherits@~2.0.1
    • escodegen@~1.3.2
      • esutils@~1.0.0
      • estraverse@~1.5.0
      • esprima@~1.1.1
      • source-map@~0.1.33
        • amdefine@>=0.0.4
    • falafel@^1.0.0
      • acorn@^1.0.3
      • foreach@^2.0.5
      • isarray@0.0.1
      • object-keys@^1.0.6
    • has@^1.0.0
      • function-bind@^1.0.2
    • object-inspect@~0.4.0
    • quote-stream@~0.0.0
      • through2@~0.4.1
        • readable-stream@~1.0.17
          • core-util-is@~1.0.0
          • isarray@0.0.1
          • string_decoder@~0.10.x
          • inherits@~2.0.1
        • xtend@~2.1.1
          • object-keys@~0.4.0
      • minimist@0.0.8
    • readable-stream@~1.0.27-1
      • core-util-is@~1.0.0
      • isarray@0.0.1
      • string_decoder@~0.10.x
      • inherits@~2.0.1
    • shallow-copy@~0.0.1
    • static-eval@~0.2.0
      • escodegen@~0.0.24
        • esprima@~1.0.2
        • estraverse@~1.3.0
        • source-map@>= 0.1.2
          • amdefine@>=0.0.4
    • through2@~0.4.1
      • readable-stream@~1.0.17
        • core-util-is@~1.0.0
        • isarray@0.0.1
        • string_decoder@~0.10.x
        • inherits@~2.0.1
      • xtend@~2.1.1
        • object-keys@~0.4.0
  • through2@^2.0.0
    • readable-stream@~2.0.0
      • core-util-is@~1.0.0
      • inherits@~2.0.1
      • isarray@0.0.1
      • process-nextick-args@~1.0.0
      • string_decoder@~0.10.x
      • util-deprecate@~1.0.1
    • xtend@~4.0.0
• duplexify@^3.2.0
  • end-of-stream@1.0.0
    • once@~1.3.0
      • wrappy@1
  • readable-stream@^2.0.0
    • core-util-is@~1.0.0
    • inherits@~2.0.1
    • isarray@0.0.1
    • process-nextick-args@~1.0.0
    • string_decoder@~0.10.x
    • util-deprecate@~1.0.1
• glob-stream@^5.0.0
  • extend@^3.0.0
  • glob@^5.0.3
    • inflight@^1.0.4
      • once@^1.3.0
        • wrappy@1
      • wrappy@1
    • inherits@2
    • minimatch@2 || 3
      • brace-expansion@^1.0.0
        • balanced-match@^0.3.0
        • concat-map@0.0.1
    • once@^1.3.0
      • wrappy@1
    • path-is-absolute@^1.0.0
  • glob2base@^0.0.12
    • find-index@^0.1.1
  • minimatch@^2.0.1
    • brace-expansion@^1.0.0
      • balanced-match@^0.3.0
      • concat-map@0.0.1
  • ordered-read-streams@^0.3.0
    • is-stream@^1.0.1
    • readable-stream@^2.0.1
      • core-util-is@~1.0.0
      • inherits@~2.0.1
      • isarray@0.0.1
      • process-nextick-args@~1.0.0
      • string_decoder@~0.10.x
      • util-deprecate@~1.0.1
  • through2@^0.6.0
    • readable-stream@>=1.0.33-1 <1.1.0-0
      • core-util-is@~1.0.0
      • isarray@0.0.1
      • string_decoder@~0.10.x
      • inherits@~2.0.1
    • xtend@>=4.0.0 <4.1.0-0
  • to-absolute-glob@^0.1.1
    • extend-shallow@^2.0.1
      • is-extendable@^0.1.0
  • unique-stream@^2.0.2
    • through2-filter@^2.0.0
      • through2@~2.0.0
        • readable-stream@~2.0.0
          • core-util-is@~1.0.0
          • inherits@~2.0.1
          • isarray@0.0.1
          • process-nextick-args@~1.0.0
          • string_decoder@~0.10.x
          • util-deprecate@~1.0.1
        • xtend@~4.0.0
      • xtend@~4.0.0
• graceful-fs@^4.0.0
• gulp-sourcemaps@^1.5.2
  • convert-source-map@^1.1.1
  • graceful-fs@^4.1.2
  • strip-bom@^2.0.0
    • is-utf8@^0.2.0
  • through2@^2.0.0
    • readable-stream@~2.0.0
      • core-util-is@~1.0.0
      • inherits@~2.0.1
      • isarray@0.0.1
      • process-nextick-args@~1.0.0
      • string_decoder@~0.10.x
      • util-deprecate@~1.0.1
    • xtend@~4.0.0
  • vinyl@^1.0.0
    • clone@^1.0.0
    • clone-stats@^0.0.1
    • replace-ext@0.0.1
• is-valid-glob@^0.3.0
• merge-stream@^1.0.0
  • readable-stream@^2.0.1
    • core-util-is@~1.0.0
    • inherits@~2.0.1
    • isarray@0.0.1
    • process-nextick-args@~1.0.0
    • string_decoder@~0.10.x
    • util-deprecate@~1.0.1
• mkdirp@^0.5.0
  • minimist@0.0.8
• object-assign@^4.0.0
• strip-bom@^2.0.0
  • is-utf8@^0.2.0
• strip-bom-stream@^1.0.0
  • first-chunk-stream@^1.0.0
  • strip-bom@^2.0.0
    • is-utf8@^0.2.0
• through2@^2.0.0
  • readable-stream@~2.0.0
    • core-util-is@~1.0.0
    • inherits@~2.0.1
    • isarray@0.0.1
    • process-nextick-args@~1.0.0
    • string_decoder@~0.10.x
    • util-deprecate@~1.0.1
  • xtend@~4.0.0
• through2-filter@^2.0.0
  • through2@~2.0.0
    • readable-stream@~2.0.0
      • core-util-is@~1.0.0
      • inherits@~2.0.1
      • isarray@0.0.1
      • process-nextick-args@~1.0.0
      • string_decoder@~0.10.x
      • util-deprecate@~1.0.1
    • xtend@~4.0.0
  • xtend@~4.0.0
• vinyl@^1.0.0
  • clone@^1.0.0
  • clone-stats@^0.0.1
  • replace-ext@0.0.1

That's some food for thought.

[jbenet]

[jbenet]

i dont even