Replace callbacks with promises #12
Conversation
SemenchenkoVitaliy
force-pushed
the
replace-callbacks-with-promises
branch
from
713cb9f to
1bb1c97
Compare
|
Blocked until metarhia/common#290. |
| } | ||
|
|
||
| const stats = await fs.stat(file); | ||
| if (stats.size <= minCompressSize) return false; |
There was a problem hiding this comment.
Unrelated to this PR, but:
Now that I think of it, wouldn't this lead for the user of Filestorage to a need to keep a map of compressed/uncompressed files? Because without such map if someone calls compress(file, 100, 'xz') and the file size is less than 100 then the file will not be compressed and if the user then calls read(file, { compression: 'xz' }) this would lead to an error as the file was not compressed?
There was a problem hiding this comment.
The idea was that metadata including file name, hash sum, deduplication hash, compression type etc. will be stored in the DB. I don't see a problem with the need for such a map since you already have to store files compression types somewhere.
SemenchenkoVitaliy
force-pushed
the
replace-callbacks-with-promises
branch
from
59bceb0 to
7d19dc7
Compare
SemenchenkoVitaliy
force-pushed
the
replace-callbacks-with-promises
branch
from
7d19dc7 to
e4ccff5
Compare
lib/fs.js
Outdated
| 'readdir', | ||
| ]; | ||
|
|
||
| if (process.version.slice(1).split('.')[0] >= 10) { |
There was a problem hiding this comment.
| if (process.version.slice(1).split('.')[0] >= 10) { | |
| if (fs.promises) { |
There was a problem hiding this comment.
I think we should even check for Node.js '11.14.0' (nodejs/node#26581) in here to avoid having the warning in the console every time this is run.
lib/filestorage.js
Outdated
| // checksum - <string>, data checksum | ||
| // dedupHash - <string>, second data checksum | ||
| // size - <number>, data size | ||
| // Returns: <Object>, stats |
There was a problem hiding this comment.
It returns a Promise, since it is an AsyncFunction.
There was a problem hiding this comment.
Yes, but we need to somehow specify what this promise resolves.
There was a problem hiding this comment.
@SemenchenkoVitaliy we don't have a special syntax in metadoc for that, at the moment most of the projects just specify the return value as Promise without providing any info on what value it resolves with (except for the function's description above), so I think you can either add some syntax for that in metadoc, or leave out the details on what this promise resolves with. But you still have to specify the correct return type, which is Promise, not Object.
lib/utils.js
Outdated
| throw new Error(`Unknown compression type ${opts.compression} specified`); | ||
| } | ||
|
|
||
| return new Promise((res, rej) => { |
There was a problem hiding this comment.
The naming is not very obvious here:
| return new Promise((res, rej) => { | |
| return new Promise((resolve, reject) => { |
| .once('error', rej) | ||
| .once('finish', () => { | ||
| if (opts.encoding) res(buffers.join('')); | ||
| else res(Buffer.concat(buffers)); | ||
| }) | ||
| .on('entry', (header, stream, next) => { |
There was a problem hiding this comment.
I don't think that this will work with gzip stream, since it doesn't emit 'entry' event:
Note: tar, tgz and zip have the same uncompressing streaming API as above: it's a writable stream, and entries will be emitted while uncompressing one after one another, while that of gzip is slightly different: gzip.UncompressStream is a transform stream, so no entry event will be emitted and you can just pipe to another stream
Also, I think, you should just use the newly added common.MemoryWritable here.
@metarhia/globalstorage changed its API from callbacks to promises in metarhia/globalstorage#416. In order to also update
FsProvider, @metarhia/filestorage should also be updated.