doc: add correct argument types for fs.cp APIs

[jasnell]

The documentation was missing the fact that the src and dest arguments for fs.cpSync, etc can be string, URL, or Buffer, and that the filter option callback can be passed a string, URL, or Buffer.

[dario-piotrowicz]

Hey @jasnell, I am so very sorry for putting a request changes feedback, I hope it's not out of line 🙇

I was just checking things and am no longer convinced that the changes here are accurate so I was hoping we could sort it out before landing this 🙇

My issue being with the parameter types for the filter functions.

As far as I can tell there is a conversion step for src and dest so that they are always converted to strings when processed by cp or cpSync, resulting in the fact that the two functions behave exactly the same regardless of the type of the paths provided (e.g. cp does not care nor change its behavior if I pass URLs to it).

Furthermore before the filtering function is called the items are computed using join which output is a string, making me believe that the args that the filtering functions get are indeed always of type string.

Example:

From the code this looks to be deliberate, do you believe it to be a bug?

[dario-piotrowicz]

@jasnell Could you also share an example of a valid use of Buffer as argument for cp/cpSync? 🙏

[jasnell]

Looks like there's some discrepancy in the implementation that I would consider a definite bug.

The bottom line is that with all file system operations we have to support the ability to work with paths as either strings or Buffers at a minimum, and ideally as URLs as well, and it looks like the current implementation of cp... is not correctly handling this. We likely shouldn't change this implementation until that bug is fixed.

For an example case of why this is important... which I alluded to earlier. Years ago I was working with a customer based in Japan that was working on migrating data files from a legacy system to a new system. Their app would read files off the disk, process their contents, and transform those into the updated data format. However, whenever they would call fs.readdir(...) to get a listing of the files in a directory, as they started to iterate over the names of the files they were getting file not found errors thrown which didn't make any sense since readdir() only returns the names of files that do actually exist. The files were not being deleted so it was not a race condition. Turns out, however, that some of the file names in the directory were Shift-JIS encoded rather than UTF8 so readdir() was actually mangling those file names so that what the application saw was completely wrong. It looks like our existing cp sync method has the same kind of problem.

[jasnell]

Bug filed: #58634

Specifically, when passing a Buffer the API throws.
When passing a URL it coerces to a string then appends the file name, which is also invalid because it is not correctly handling the path normalization. The results might be parseable but that doesn't make them correct.

[dario-piotrowicz]

I see, thanks a lot for the clarification here 🫶

Based on the above I feel that updating the docs at this time would be incorrect since we'd be documenting something that is different to node's actual behavior (even if the current implementation is not fully correct).

My personal preference in this case would be to keep the docs as they are until #58634 is addressed, wdyt? 🙂