Implement second pipe function to 'download' from nextcloud #10
Conversation
ghost
requested review from
source/webdav.ts
Outdated
| export const getReadStream = clientFunction(rawGetReadStream); | ||
| export const touchFolder = clientFunction(rawTouchFolder); | ||
|
|
||
| export const pipeStream = clientFunction(rawUpload); // deprecated |
There was a problem hiding this comment.
We should probably emit a warning?! How would one do that in a library ?
There was a problem hiding this comment.
There was a problem hiding this comment.
Thought the same. Will do process.emitWarning
tests/webdav-jest.ts
Outdated
| describe('pipeStream(path, stream)', () => { | ||
| it('should pipe readable streams to the Nextcloud instance', async () => { | ||
| describe('upload(targetPath, readStream)', () => { | ||
| it('should pipe data to the Nextcloud instance', async () => { |
There was a problem hiding this comment.
Well, it's not wrong that upload and download both still expect streams, is it?
tests/webdav-jest.ts
Outdated
|
|
||
| expect(await client.get(path)).toBe(string); | ||
|
|
||
| await client.remove(path); | ||
| }); | ||
| }); | ||
|
|
||
| fdescribe('download(sourcePath, writeStream)', () => { |
There was a problem hiding this comment.
thanks, forgot to remove the focus
source/types.ts
Outdated
| @@ -30,7 +30,9 @@ export interface NextcloudClientInterface extends NextcloudClientProperties { | |||
| getFolderProperties(path: string, extraProperties?: FileDetailProperty[]): Promise<FolderProperties>; | |||
| configureWebdavConnection(options: ConnectionOptions): void; | |||
| configureOcsConnection(options: ConnectionOptions): void; | |||
| pipeStream(path: string, stream: Stream.Readable): Promise<void>; | |||
| pipeStream(path: string, readStream: Stream.Readable): Promise<void>; | |||
| upload(targetPath: string, readStream: Stream.Readable): Promise<void>; | |||
There was a problem hiding this comment.
What about uploadFromStream and downloadToStream ?
Or, to stay with webdav terms:
postFromStream and getAsStream
There was a problem hiding this comment.
uploadFromStream and downloadToStream sound ok
|
Do you think just removing the function would be better? Sure, that requires amajor version bump but it might be worth it given the small adoption the library has |
I simply didn't think that this should make it a nextcloud-link v2.0.0… from a semver POV, yes, it could also be done like that, but feels odd. |
|
I could add a deadline when I'll remove the function alltogether |
Yeah, you could say something along the line of "this will be removed in version 2" |
|
Yess, update incoming |
ghost
closed this
added function to pipe data located at the provided path into the provided writable stream
renamed
pipeStreamtouploadFromStreamanddownloadToStreamI have carefully evaluated the risks of this change
This change does not remove existing functionality
I have written tests to cover all risks
I updated the docs