Conversation
joshwcomeau
left a comment
joshwcomeau
left a comment
There was a problem hiding this comment.
Awesome! This looks really great, and the strategy you came up with is brilliant. I'm not sure my solution would have been as smart, using fs.stat to find the modified date of all components in the dir.
Also, thanks for using rimraf! I wouldn't have thought to use it, but I think it's really good for multiplatform-support.
I have a few nits and a couple changes requested. You don't have to do all of them, some of them are just suggestions, but I think we should either discuss or implement:
- the various quick nits/copy change
- Changing the convention for the prompt to
y/N - Implementing the 60-second no-prompt-for-delete
- Refactoring the date-checking function to return the file data rather than use an outside variable, and using .sort to find the newest one in the
Promise.all
Thanks so much for your time on this :)
| module.exports.logConclusion = () => { | ||
| console.info('\n'); | ||
| console.info(chalk.bold.rgb(...colors.green)('Component created! 🚀 ')); | ||
| console.info(chalk.rgb(...colors.blue)(`If this was a mistake, run ${chalk.bold`new-component-undo`}`)); |
There was a problem hiding this comment.
Can we change this copy to "Made a mistake? Undo this component by running 'new-component-undo'."
| // Delete component directory | ||
| readDirPromise(config.dir).then(files => { | ||
| if (files.length === 0) { | ||
| console.log(`There\'s no files in ${config.dir}! Is this where your components |
There was a problem hiding this comment.
Nit: I don't think the escaped single-quote is necessary, since you're using backticks.
| lastCreatedFile = { name, dateModified }; | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Clever solution! Never used fs.stat before, looks perfect for this.
One small note: what do you think about the statPromise.then returning the lastCreatedFile object? That way, after the Promise.all resolves, you'll have an array of lastCreatedFile objects, and you can do a .sort() on the array to find the earliest one, using the dateModified as the comparator.
I think the benefit is you can make these inner functions a bit more pure, where they aren't depending on the parent scope's lastCreatedFile variable. It's not a huge deal because this is all enclosed in a pure function anyway, but I think it'd be slightly nicer :)
There was a problem hiding this comment.
I didn't like this solution, I was thinking for a neater solution too, this sounds good to me.
| Promise.all(filePromises).then(() => { | ||
| const componentName = lastCreatedFile.name; | ||
| // Log the intro | ||
| logUndoConfirmation(componentName); |
There was a problem hiding this comment.
Hm... What do you think about doing a check, to see if the component was made in the last 60 seconds, and skipping the confirmation if it was?
The times that i've wanted an "undo", I realize almost instantly, and I haven't even touched the generated files yet. So I think it would be nice to skip the confirmation if less than a minute has passed.
This would require a bit of a rewrite, to separate the "asking" from the "deleting", but I think this is a good refactor to do anyway :)
| process.stdin.setRawMode(true); | ||
| } else { | ||
| tty.setRawMode(true); | ||
| } |
There was a problem hiding this comment.
Why do we need to set raw mode? I don't know much about it but a quick google says that it just handles dealing character-by-character, and disables shortcuts like CMD-C?
|
|
||
| module.exports.logUndoConfirmation = (name) => { | ||
| console.info(`This will delete the component '${chalk.bold(name)}' and all files within the directory.`); | ||
| console.info(chalk.rgb(...colors.blue)(`Press ${chalk.bold`Enter`} to continue, or ${chalk.bold`any other key to cancel`}.`)); |
There was a problem hiding this comment.
What do you think about using the convention of y/N? Not sure if you've seen that, but you have to enter either 'y' for yes, or 'n' for no. It's case-insensitive, and the capital N indicates that if you just press 'enter', that will be selected. I think maybe any other key will also opt for the default N option.
You'll see below I suggest not prompting the user for components made in the last minute - because this will now be prompting for slightly older components, I think it's wise to make it a bit harder to delete a component :)
| } | ||
| process.stdin.resume(); | ||
| process.stdin.on('keypress', function(str, key) { | ||
| if (key && key.name !== 'return') process.exit(0); |
There was a problem hiding this comment.
Nit: as nice as they look, I've been burned by single-line if statements. Can we use blocks and format it this way?
if (key && key.name !== 'return') {
process.exit(0);
}It'd be nice to add Prettier to this project, so that styling concerns are taken care of :) I'll open an issue for it
| console.error(err); | ||
| }); | ||
| }) | ||
| } |
There was a problem hiding this comment.
I talked a little about it above, but I think it'd be nice if this was two smaller functions, and they could be composed (in the loose sense of the word):
const promptForDeletion = callback => componentName => {
// prompt setup left out because I'm lazy
process.stdin.on('keypress', () => {
if (key !== 'y') {
process.exit(0);
}
callback(componentName);
});
}
const deleteComponent = (componentName) => {
rimrafPromise(... rest of your code here)
}
const promptAndDelete = promptForDeletion(deleteComponent);I may be getting too fancy with the currying. A more traditional approach:
const promptForDeletion = (callback, componentName) => {
// stuff
};
const promptAndDelete = componentName => (
promptForDeletion(deleteComponent, componentName)
);Up to you which you prefer :)
| err ? reject(err) : resolve(file); | ||
| }); | ||
| }) | ||
| ); |
There was a problem hiding this comment.
Thanks for these helpers!!
If you wanted to be really fancy, you could make a "promise wrapper" helper. Something like:
const wrapInPromise = fn => (...args) => (
new Promise((resolve, reject) => {
fn(...args, (err, ...data) => {
err ? reject(err) : resolve(...data);
});
})
);
module.exports.readDirPromise = wrapInPromise(fs.readdir);
module.exports.rimrafPromise = wrapInPromise(rimraf);
module.exports.statPromise = wrapInPromise(fs.stat);You don't have to do this if you don't want though :) I'm not sure it's actually helpful, may be too clever for its own good. Plus then you'd have to specify the character encoding as UTF-8 when you use readDirPromise, since there's no way to automatically provide it.
(Also I'm not sure if Node natively supports rest/spread, so it may be impossible anyway?)
|
What happened to this PR? Is this gonna be merged? Everything looked good by your discussion. I'm happy to help if you need any changes @joshwcomeau! |
3 participants
Hi, @joshwcomeau!
I added
new-component-undofor now. Take a look and then we can discuss the command format :)Thanks!