feat: support iterables in file#save (#2202)

[matthieusieben]

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

• Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
• Ensure the tests and linter pass
• Code coverage does not decrease (if any source code was changed)
• Appropriate docs were updated (if necessary)

Fixes #2202 🦕

BEGIN_COMMIT_OVERRIDE
feat: support iterables in file@save
END_COMMIT_OVERRIDE

[ddelgrosso1]

@matthieusieben thank you for the contribution, we always appreciate getting them. Let me check regarding the disabling of retries but I think we will have to find another way around as this would not adhere to the documentation and would probably be pretty specific to Node storage when compared to our other languages.

[matthieusieben]

Here is a summary of the changes that were added:

• Do not attempt to read a destroyed readable as this would yield a never resolving promise
• Do not leave the promise "hanging" when we bail (reject will always be called, regardless of wether bail was called)
• Do not retry when a failure occurs with a readable stream or (async) iterable
• Properly cleanup if data is not valid and causes pipeline to throw before the pipeline is actually running
• Added more test cases

[ddelgrosso1]

I think this is looking pretty good. Looks like some issues with tests on Node 12. Ideally I would like to get this in before the move to 14.

[matthieusieben]

The reason node-12 is failing is that pipeline only supported Streams (and not AsyncIterable, Iterable or PipelineSourceFunction). Not only did node12 not support those but it does not even throw a proper error in theses cases.

https://nodejs.org/docs/latest-v12.x/api/stream.html#stream_stream_pipeline_streams_callback

How should I adapt to this limitation of Node 12 ?

[matthieusieben]

One approach would be to add the following when data is not a buffer or string

// In node 12, pipeline() only supports "Stream"s
if (!('pipe' in data) || typeof data.pipe !== 'function') {
  const nodeVersion = Number(process.versions.node.split('.')[0]);
  if (!nodeVersion || nodeVersion < 14) {
    throw new Error('Saving using an iterable requires Node 14 (or higher)');
  }
}

And to catch that particular error in tests. What do you think ?

[ddelgrosso1]

Removed do not merge as 7.0 is live.

[ddelgrosso1]

BEGIN_COMMIT_OVERRIDE
feat: support iterables in file@save
END_COMMIT_OVERRIDE