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

Stabilize fs.cp(), fs.cpSync(), fsPromises.cp() methods #44598

Closed
theoludwig opened this issue Sep 11, 2022 · 8 comments · Fixed by #53127
Closed

Stabilize fs.cp(), fs.cpSync(), fsPromises.cp() methods #44598

theoludwig opened this issue Sep 11, 2022 · 8 comments · Fixed by #53127
Labels
experimental Issues and PRs related to experimental features. feature request Issues that request new features to be added to Node.js. fs Issues and PRs related to the fs subsystem / file system.

Comments

@theoludwig
Copy link
Contributor

theoludwig commented Sep 11, 2022

What is the problem this feature will solve?

Currently, these methods are marked as Experimental (Stability: 1):

There were no major changes since their introduction in Node.js v16.7.0, so it should be safe to mark them Stable (Stability: 2) as per the Stability index.

If somehow, these methods are still not ready to be stable, this is a tracking issue/roadmap to know what are the needed TODOs before marking them as stable.

What is the feature you are proposing to solve the problem?

Marking fs.cp(), fs.cpSync(), fsPromises.cp() methods as stable (Stability: 2).

For automated test purposes, mocking fs module is a must.
Currently, the most used userland library for that is probably mock-fs, and doesn't support these methods.
Before marking this API stable, Node.js should provide the needed feature(s) for mocking this API in userland, or even better, include mock-fs in Node.js core, this can be well suited with the new test_runner module.

Related issues: tschaub/mock-fs#358, #37746.

What alternatives have you considered?

I think it's best to avoid using experimental APIs, avoid warnings, and simply use features subject to semantic-versioning for easier Node.js upgrades.

The current workaround, I personally use and end up copy/pasting in several projects (too small to be an npm package IMO):

import fs from 'node:fs'
import path from 'node:path'

export const copyDirectory = async (
  source: string,
  destination: string
): Promise<void> => {
  const filesToCreate = await fs.promises.readdir(source)
  for (const file of filesToCreate) {
    const originalFilePath = path.join(source, file)
    const stats = await fs.promises.stat(originalFilePath)
    if (stats.isFile()) {
      const writePath = path.join(destination, file)
      await fs.promises.copyFile(originalFilePath, writePath)
    } else if (stats.isDirectory()) {
      await fs.promises.mkdir(path.join(destination, file))
      await copyDirectory(path.join(source, file), path.join(destination, file))
    }
  }
}

By marking this API as stable, I should be able to use it like this:

import fs from 'node:fs'

await fs.promises.cp(source, destination, { recursive: true })
@theoludwig theoludwig added the feature request Issues that request new features to be added to Node.js. label Sep 11, 2022
@VoltrexKeyva VoltrexKeyva added the fs Issues and PRs related to the fs subsystem / file system. label Sep 11, 2022
@tniessen tniessen added the experimental Issues and PRs related to experimental features. label Sep 12, 2022
@aduh95
Copy link
Contributor

aduh95 commented Sep 15, 2022

@nodejs/fs

@piedar
Copy link

piedar commented Oct 25, 2022

👍 This is a useful API, but the documentation regarding recursive directory copies should be clarified. It currently states

When copying a directory to another directory, globs are not supported and
behavior is similar to cp dir1/ dir2/.

But this is not quite right, as cp -r dir1/ dir2/ copies dir1 into dir2 like ./dir2/dir1/. This command is actually more like cp -r --no-target-directory dir1/ dir2/ which copies the contents of dir1 into dir2.

@theoludwig
Copy link
Contributor Author

Possible to stabilize them before v20? #47381

@FStefanni
Copy link

Hi,

just to report a strange behavior, I am not sure if it is intended or a bug.
When copy a file to a directory:

fsPromises.cp(
      sourceFile,
      destinationDir,
      {"recursive": true}
);

I get this error:

  code: 'ERR_FS_CP_NON_DIR_TO_DIR',
  info: {
    message: 'cannot overwrite non-directory /tmp/my-file.txt with directory /home/myuser',
    path: '/home/myuser',
    syscall: 'cp',
    errno: 20,
    code: 'ENOTDIR'
  },

To fix, I have to use:

fsPromises.cp(
      sourceFile,
      destinationDir + "/dstFileName.txt",
      {"recursive": true}
);

I was expecting a behavior basically as close as possible to the standard bash cp command... This is weird.
Also, the message seems misleading, since it seems to do the opposite of the intended behavior.

My node is v20.2.0.

Regards

Copy link
Contributor

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Nov 22, 2023
@theoludwig
Copy link
Contributor Author

Still relevant, as it's still not stable yet.

@github-actions github-actions bot removed the stale label Nov 23, 2023
Copy link
Contributor

There has been no activity on this feature request for 5 months. To help maintain relevant open issues, please add the never-stale Mark issue so that it is never considered stale label or close this issue if it should be closed. If not, the issue will be automatically closed 6 months after the last non-automated comment.
For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label May 21, 2024
@theoludwig
Copy link
Contributor Author

Should be worth adding the never-stale label to this issue, as the issue is relevant until these fs methods are marked as stable.

@github-actions github-actions bot removed the stale label May 22, 2024
fahrradflucht added a commit to fahrradflucht/node that referenced this issue May 25, 2024
The error messages for `ERR_FS_CP_DIR_TO_NON_DIR` and
`ERR_FS_CP_NON_DIR_TO_DIR` were the inverse of the copy direction
actually performed.

Refs: nodejs#44598 (comment)
fahrradflucht added a commit to fahrradflucht/node that referenced this issue May 25, 2024
The error messages for `ERR_FS_CP_DIR_TO_NON_DIR` and
`ERR_FS_CP_NON_DIR_TO_DIR` were the inverse of the copy direction
actually performed.

Refs: nodejs#44598 (comment)
aduh95 pushed a commit that referenced this issue May 26, 2024
PR-URL: #53127
Fixes: #44598
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
nodejs-github-bot pushed a commit that referenced this issue May 31, 2024
The error messages for `ERR_FS_CP_DIR_TO_NON_DIR` and
`ERR_FS_CP_NON_DIR_TO_DIR` were the inverse of the copy direction
actually performed.

Refs: #44598 (comment)
PR-URL: #53150
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Feng Yu <F3n67u@outlook.com>
targos pushed a commit that referenced this issue Jun 1, 2024
PR-URL: #53127
Fixes: #44598
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
targos pushed a commit that referenced this issue Jun 1, 2024
The error messages for `ERR_FS_CP_DIR_TO_NON_DIR` and
`ERR_FS_CP_NON_DIR_TO_DIR` were the inverse of the copy direction
actually performed.

Refs: #44598 (comment)
PR-URL: #53150
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Feng Yu <F3n67u@outlook.com>
eliphazb pushed a commit to eliphazb/node that referenced this issue Jun 20, 2024
PR-URL: nodejs#53127
Fixes: nodejs#44598
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
eliphazb pushed a commit to eliphazb/node that referenced this issue Jun 20, 2024
The error messages for `ERR_FS_CP_DIR_TO_NON_DIR` and
`ERR_FS_CP_NON_DIR_TO_DIR` were the inverse of the copy direction
actually performed.

Refs: nodejs#44598 (comment)
PR-URL: nodejs#53150
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Feng Yu <F3n67u@outlook.com>
bmeck pushed a commit to bmeck/node that referenced this issue Jun 22, 2024
PR-URL: nodejs#53127
Fixes: nodejs#44598
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
bmeck pushed a commit to bmeck/node that referenced this issue Jun 22, 2024
The error messages for `ERR_FS_CP_DIR_TO_NON_DIR` and
`ERR_FS_CP_NON_DIR_TO_DIR` were the inverse of the copy direction
actually performed.

Refs: nodejs#44598 (comment)
PR-URL: nodejs#53150
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Feng Yu <F3n67u@outlook.com>
marco-ippolito pushed a commit that referenced this issue Jul 19, 2024
The error messages for `ERR_FS_CP_DIR_TO_NON_DIR` and
`ERR_FS_CP_NON_DIR_TO_DIR` were the inverse of the copy direction
actually performed.

Refs: #44598 (comment)
PR-URL: #53150
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Feng Yu <F3n67u@outlook.com>
marco-ippolito pushed a commit that referenced this issue Jul 19, 2024
The error messages for `ERR_FS_CP_DIR_TO_NON_DIR` and
`ERR_FS_CP_NON_DIR_TO_DIR` were the inverse of the copy direction
actually performed.

Refs: #44598 (comment)
PR-URL: #53150
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Feng Yu <F3n67u@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
experimental Issues and PRs related to experimental features. feature request Issues that request new features to be added to Node.js. fs Issues and PRs related to the fs subsystem / file system.
Projects
Development

Successfully merging a pull request may close this issue.

6 participants