fix(fslib): ensure path is a string before calling replace().

[nmoinvaz]

What's the problem this PR addresses?

Random error when calling yarn run build on Windows 11, node 17.6.0, and yarn 3.2.0.

Failed to compile.

p.replace is not a function

Debugging the issue, here is the value of p and also the stack trace during the failure.

p = 
<Buffer 5c 5c 3f 5c 43 3a 5c 55 73 65  ... 10 more bytes>
Trace: 
    at Object.toPortablePath (.pnp.cjs:20132:15)
    at PosixFS.mapToBase (.pnp.cjs:22644:18)
    at PosixFS.rmdirSync (.pnp.cjs:22561:39)
    at URLFS.rmdirSync (.pnp.cjs:22561:24)
    at _rmdirSync (node:internal/fs/rimraf:260:21)
    at rimrafSync (node:internal/fs/rimraf:193:7)
    at node:internal/fs/rimraf:253:9
    at Array.forEach (<anonymous>)
    at _rmdirSync (node:internal/fs/rimraf:250:7)
    at rimrafSync (node:internal/fs/rimraf:193:7)

How did you fix it?

path in fs.rmdirSync and other Node functions can be string | Buffer | URL. So call .toString() before calling .replace().

Checklist

• I have read the Contributing Guide.

• I have set the packages that need to be released for my changes to be effective.

• I will check that all automated PR checks pass before the PR gets reviewed.

[merceyz]

Please add a test to https://github.com/yarnpkg/berry/blob/c9ddb263c1c003c6df9cbcaf4e427847edca0d35/packages/yarnpkg-fslib/tests/npath.test.ts

[nmoinvaz]

Any idea why the CI fails?

[nmoinvaz]

Test added.

[arcanis]

LGTM

[merceyz]

I'm not sure if this is safe to land so I'll defer to @paul-soporan since he looked into Buffer support in #1584 and knows more about what is required.

Ref #1818

Any idea why the CI fails?

A flaky test, you can ignore it.

[nmoinvaz]

Anything else on this commit to do?

[paul-soporan]

I apologize for the delay, I'll try doing an in-depth review tomorrow.

[paul-soporan]

Haven't had time to look into the encoding issues yet, but, regarding the API, toPortablePath isn't the right place for this change. It's only meant to accept a Path.

Introducing an invisible layer here means that things might break (Edit: Actually, in this specific case they wouldn't, but still, it gives off the false impression that we fully support Buffers without them being implemented in the right layer) when going through the FS classes that only expect paths.

The right fix would be to create a BufferFS to automatically wrap the patchedFs just like the URLFS does.

[nmoinvaz]

I have made the suggested changes and rebased.

[paul-soporan]

I'm sorry if that wasn't clear - I meant to say that we should create a BufferFS class inside a new BufferFS file (just like URLFS is implemented), not modify PosixFS. The patchFs function should then be modified to wrap the existing fs with a BufferFS too (

berry/packages/yarnpkg-fslib/sources/patchFs.ts

Lines 146 to 147 in e143651

	// We wrap the `fakeFs` with a `URLFS` to add support for URL instances
	fakeFs = new URLFS(fakeFs);

).

[nmoinvaz]

I have closed the pull request. I don't want to see it through.

My workaround is just to delete the build directory before running yarn build.