2.0 - refactor #8
Conversation
|
@doowb please review when you have a chance. |
| return writeFile.promise.apply(null, arguments); | ||
| } | ||
| const opts = { encoding: 'utf8', ...options }; | ||
| const destpath = opts.increment ? incrementName(filepath) : filepath; |
There was a problem hiding this comment.
Great idea! I like that you're doing the check here and incrementing here. It makes it simple later on to just do a file exists check.
| * @name write | ||
| * @param {String} `filepath` file path. | ||
| * @param {String|Buffer|Uint8Array} `data` Data to write. | ||
| * @param {Object} `options` Options to pass to [fs.writeFile][writefile] | ||
| * @param {Function} `callback` (optional) If no callback is provided, a promise is returned. | ||
| * @api public | ||
| */ |
There was a problem hiding this comment.
These comments could use some information on the return value. I noticed there's a result object with path and data returned. I think this is a good idea, especially when increment is used.
index.js
Outdated
| * @param {String} `filepath` file path. | ||
| * @param {String|Buffer|Uint8Array} `data` Data to write. | ||
| * @param {Object} `options` Options to pass to [fs.writeFileSync][writefilesync] | ||
| * @return {undefined} |
There was a problem hiding this comment.
The return value is an object with path and data.
index.js
Outdated
| * @api public | ||
| */ | ||
|
|
||
| writeFile.sync = function(filepath, data, options) { | ||
| write.stream = (filepath, contents, options) => { |
There was a problem hiding this comment.
contents isn't used in this method.
| writeFile.stream = function(filepath, options) { | ||
| mkdirp.sync(path.dirname(filepath), options); | ||
| return fs.createWriteStream(filepath, options); | ||
| const incrementName = destpath => { |
index.js
Outdated
| // data is a buffer, we only need to call `.toString()` on | ||
| // the entire string if the condition is true. | ||
| if (String(data.slice(-1)) !== '\n') { | ||
| return data.toString() + '\n'; |
There was a problem hiding this comment.
It should be possible to append a newline to the buffer without doing .toString().
There was a problem hiding this comment.
nice, good catch! I wasn't thinking about that. This is an awesome example of a place where we can make code much more performant with a really simple optimization. Awesome.
There was a problem hiding this comment.
I changed it to:
if (String(data.slice(-1)) !== '\n') {
if (typeof data === 'string') {
return data + '\n';
}
return data.concat(Buffer.from('\n'));
}There was a problem hiding this comment.
I think this should be possible with other things too, like front matter. I'll look into it.
There was a problem hiding this comment.
@doowb I just saw your point below about the Buffer class. LMKWYT.
| // if filepath !== destpath, that means the user has enabled | ||
| // "increment", which has already checked the file system and | ||
| // renamed the file to avoid conflicts, so we don't need to | ||
| // check again. |
There was a problem hiding this comment.
✅ I was wondering about this above.
index.js
Outdated
| }; | ||
|
|
||
| const mkdir = (dirname, options) => { | ||
| return new Promise(res => fs.mkdir(dirname, options, () => res())); |
There was a problem hiding this comment.
This is just a knit pick, but everywhere else you use a Promise like this, you use resolve. I only bring it up because it took be a split second to notice was res was.
| } catch (err) { /* do nothing */ } | ||
| }; | ||
|
|
||
| const isBuffer = data => { |
There was a problem hiding this comment.
Nice... I understand that my statement above about adding the newline to the buffer would only be possible if using the built-in Buffer class. But since you're doing this isBuffer check here, it seems intentional to leave Buffer out.
There was a problem hiding this comment.
Oh, damn. Good point. Hmm, we can either include the Buffer class, which - if I recall correctly - doesn't the native fs module need the buffer class anyway? Or we can just call .toString() if it's not a string already, before appending the \n.
Thoughts?
There was a problem hiding this comment.
I think we leave Buffer in above, but we have an idea of what to do if people complain that they can't bundle write for some reason.
|
@doowb LMK if you think this is good to publish. |
|
LGTM! |
|
Thanks @doowb! |
No description provided.