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

fix: removed recursive-copy module from the production dependency #1265

Merged
merged 5 commits into from
Aug 16, 2024

Conversation

aryamohanan
Copy link
Contributor

@aryamohanan aryamohanan commented Aug 9, 2024

The module recursive-copy currently has inflight(deprecated) as a subdependency, which triggers following npm warnings during the installation of our collector.

npm WARN deprecated [email protected]: This module is not supported and has memory leaks. Avoid using it. Consider lru-cache for a more robust and tested solution for coalescing async requests by key value.

To resolve this issue, we are removing recursive-copy from our production dependencies and will instead use fs-extra, which is a drop-in replacement for the native fs module. All methods in fs are included in fs-extra.

While I generally prefer to avoid external libraries and additional dependencies, and would prefer to copy directories recursively using native fs methods, the fs.cp function was not available in earlier versions of Node.js 16.

Note: fs.cp and fsPromises.cp, which can copy directories, are available in Node v16.7.0. However, since we also support Node v14, we will not use these methods at this time. Instead, we’ll implement a custom solution using fs-extra methods to ensure compatibility with our existing Node.js versions. In the next major version we can consider to replace it with native fs.cp method. The syntax is very similar.

Reasons to choose fs-extra:

  • The methods closely resemble those of the native fs module, making migration straightforward.
  • With the last commit being 5 months ago.
  • Favorable comparison on npm trends: fs-extra vs ncp vs recursive-copy.
  • Compatible with Node.js v14.
  • Alternatives like ncp and wrench are no longer maintained. Thus, fs-extra is likely the best option. The Developer of Wrench directs users to use fs-extra as he has deprecated his library.

refs : INSTA-12493

@aryamohanan aryamohanan changed the title Replace recursive copy fix: removed recursive-copy module from the production dependency Aug 9, 2024
@aryamohanan aryamohanan force-pushed the replace-recursive-copy branch 7 times, most recently from bf78260 to f574ec6 Compare August 13, 2024 08:16
@aryamohanan aryamohanan force-pushed the replace-recursive-copy branch from 9640821 to 35be99a Compare August 13, 2024 11:15
@aryamohanan aryamohanan force-pushed the replace-recursive-copy branch 5 times, most recently from bd07e06 to c7728c3 Compare August 13, 2024 15:55
@aryamohanan aryamohanan force-pushed the replace-recursive-copy branch from c7728c3 to b988d7e Compare August 13, 2024 16:14
@aryamohanan aryamohanan marked this pull request as ready for review August 14, 2024 04:59
@aryamohanan aryamohanan requested a review from a team as a code owner August 14, 2024 04:59
@kirrg001
Copy link
Contributor

Note: fs.cp and fsPromises.cp, which can copy directories, are available in Node v16.7.0. However, since we also support Node v14

refs #848

Maybe its worth adding a task to the v4 release to replace fs-extra with the native fs.cp?

Copy link
Contributor

@kirrg001 kirrg001 left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this. Added some comments 👍

packages/collector/test/nativeModuleRetry/test.js Outdated Show resolved Hide resolved
packages/shared-metrics/package.json Show resolved Hide resolved
package-lock.json Show resolved Hide resolved
packages/collector/test/nativeModuleRetry/test.js Outdated Show resolved Hide resolved
packages/collector/test/nativeModuleRetry/test.js Outdated Show resolved Hide resolved
packages/collector/test/nativeModuleRetry/test.js Outdated Show resolved Hide resolved
@aryamohanan aryamohanan force-pushed the replace-recursive-copy branch 2 times, most recently from 78f1a60 to eeb4897 Compare August 14, 2024 11:17
@aryamohanan aryamohanan force-pushed the replace-recursive-copy branch from eeb4897 to b7a9df6 Compare August 16, 2024 11:12
@aryamohanan aryamohanan requested a review from kirrg001 August 16, 2024 12:24
Copy link
Contributor

@kirrg001 kirrg001 left a comment

Choose a reason for hiding this comment

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

Commit suggestion

fix(shared-metrics): removed recursive-copy dependency #1265
refs ...

  • resolves the following warning:
    . "npm WARN deprecated [email protected]: This module is not supported and has memory leaks. Avoid using it. Consider lru-cache for a more robust and tested solution for coalescing async requests by key value."

@aryamohanan aryamohanan merged commit 63e67f1 into main Aug 16, 2024
1 check passed
@aryamohanan aryamohanan deleted the replace-recursive-copy branch August 16, 2024 14:09
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.

2 participants