-
Notifications
You must be signed in to change notification settings - Fork 1
Hotfix: use node fs-extra to handle copying and removing files #3hk00xt #20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Task linked: CU-3hk00xt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of comments.
Please be aware of your formatting. When I first looked at this PR, nearly every line was changed for formatting.
I added a .prettierrc
and updated this branch, so can now respect that file when making formatting updates.
@@ -2,7 +2,7 @@ | |||
|
|||
import chalk from 'chalk'; | |||
import { exec } from 'child_process'; | |||
import fs from 'fs'; | |||
import fs from 'fs-extra'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're going to use a package, it should be listed as a dependency in package.json
. Although widely used, it's not safe to rely on dependencies' dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd also like to know why this is necessary beyond the fs basics? It doesn't seem like we're doing anything fancy, and I like to keep dependencies to a minimum.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's because of the move and remove functions, there are some underlying issues especially with callbacks that fs has but fs-extra solves it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seancdavis changes have been made based on your review
This PR fixes this issue stackbit-themes/stackbit-examples#17. It introduces node fs-extra to handle the following:
Copying files on all OS types
Removing files on all OS types
Remove code where we are running commands directly
Increase performance a bit