Skip to content

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

Merged
merged 10 commits into from
Oct 26, 2022

Conversation

hacktivist123
Copy link
Contributor

@hacktivist123 hacktivist123 commented Oct 24, 2022

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

@hacktivist123 hacktivist123 changed the title Hotfix: use node fs-extra to handle copying and removing files Hotfix: use node fs-extra to handle copying and removing files #3hk00xt Oct 24, 2022
@youvalv
Copy link

youvalv commented Oct 24, 2022

@hacktivist123 hacktivist123 marked this pull request as ready for review October 25, 2022 09:38
Copy link
Collaborator

@seancdavis seancdavis left a 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';
Copy link
Collaborator

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.

Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

@hacktivist123 hacktivist123 left a 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants