Skip to content
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

chore: replace rm -rf with node -e for cross-platform file deletion #501

Merged
merged 6 commits into from
Jun 13, 2024

Conversation

liwakin
Copy link
Contributor

@liwakin liwakin commented Jun 13, 2024

This PR includes two main updates:

  1. The .gitignore file has been updated to ignore the yarn cache directory. This is because the yarn cache does not need to be tracked in version control.

  2. The usage of 'rm -rf' has been replaced with 'rimraf' in the package scripts. This change ensures cross-platform compatibility.

These changes contribute to better project maintenance and cross-platform compatibility.

@liwakin liwakin changed the title Chore: Update .gitignore and scripts for better project maintenance chore: Update .gitignore and scripts for better project maintenance Jun 13, 2024
@liwakin liwakin changed the title chore: Update .gitignore and scripts for better project maintenance chore: update .gitignore and scripts for better project maintenance Jun 13, 2024
Copy link
Contributor

@arcanis arcanis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because the yarn cache does not need to be tracked in version control.

What is there for the project to gain by not checking-in the dependencies, in concrete and measurable terms?

Regarding rimraf, I'm personally not a fan of adding a third-party dependency just to remove a folder. If we needed cross compatibility we could just call node -e.

@liwakin
Copy link
Contributor Author

liwakin commented Jun 13, 2024

What is there for the project to gain by not checking-in the dependencies, in concrete and measurable terms?

Ignoring the .yarn/cache directory has two main benefits:

  • Reduced Repository Size: The .yarn/cache directory often contains hundreds of megabytes of data. By ignoring it, the repository becomes more lightweight.
  • Improved CI/CD Efficiency: A smaller repository means faster code fetch times during builds, improving overall build speed.

Regarding rimraf, I'm personally not a fan of adding a third-party dependency just to remove a folder. If we needed cross compatibility we could just call node -e.

I understand your concern about adding a third-party dependency like 'rimraf'. While using node -e is a valid approach, it might not be as readable as using 'rimraf'. An alternative could be implementing the cleanup functionality in a scripts/clean.js file, which would maintain readability and ensure cross-platform compatibility. What are your thoughts on this approach?

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
yarn.lock Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@liwakin liwakin changed the title chore: update .gitignore and scripts for better project maintenance chore: replace rm -rf with node -e for cross-platform file deletion Jun 13, 2024
package.json Outdated Show resolved Hide resolved
"corepack": "ts-node ./sources/_cli.ts",
"lint": "eslint .",
"prepack": "yarn build",
"postpack": "rm -rf dist shims",
"postpack": "run clean",
"rimraf": "node -e 'for(let i=2;i<process.argv.length;i++)fs.rmSync(process.argv[i],{recursive:true,force:true});'",
Copy link
Member

@styfle styfle Jun 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"rimraf": "node -e 'for(let i=2;i<process.argv.length;i++)fs.rmSync(process.argv[i],{recursive:true,force:true});'",
"rimraf": "node -e 'for(let arg of process.argv.slice(2))fs.rmSync(arg,{recursive:true,force:true})'",

slightly shorter

@aduh95 aduh95 enabled auto-merge (squash) June 13, 2024 20:43
@aduh95 aduh95 merged commit d4c5709 into nodejs:main Jun 13, 2024
10 checks passed
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.

5 participants