tschaub / mock-fs

Configurable mock for the fs module
https://npmjs.org/package/mock-fs
Other
911 stars 86 forks source link

Issue with `realpath` on symlink with Node 15.14.0 #321

Closed julien-f closed 3 years ago

julien-f commented 3 years ago
const mock = require('mock-fs')
mock({
  'file.txt': 'file content',
  'symlink': mock.symlink({ path: 'file.txt' })
})

require('fs').realpathSync('symlink')

Result:

node:fs:1820
        const dev = BigIntPrototypeToString(stats[0], 32);
                    ^

TypeError: BigInt.prototype.toString requires that 'this' be a BigInt
    at Number.toString (<anonymous>)
    at Object.realpathSync (node:fs:1820:21)
    at Object.<anonymous> (/tmp/index.js:7:15)
    at Module._compile (node:internal/modules/cjs/loader:1092:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1121:10)
    at Module.load (node:internal/modules/cjs/loader:972:32)
    at Function.Module._load (node:internal/modules/cjs/loader:813:14)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:76:12)
    at node:internal/main/run_main_module:17:47
3cp commented 3 years ago

That line is in nodejs source code.

https://github.com/nodejs/node/blob/87805375645f7af36fce2e59289559f746a7131d/lib/fs.js#L1837

It seems nodejs v15 has a breaking change, the numbers in stats need to be BigInt now. Probably mock-fs needs to add some conditional code to make sure to work with both nodejs <15 and nodejs >=15.

3cp commented 3 years ago

The nodejs v15 doc still says all the fields can be either number or bigint. Maybe internally it only uses bigint.

https://nodejs.org/dist/latest-v15.x/docs/api/fs.html#fs_stats_dev

stats.dev#

<number> | <bigint>
The numeric identifier of the device containing the file.
3cp commented 3 years ago

In nodejs v14, the code was

const dev = stats.dev.toString(32);
// and
const dev = stats[0].toString(32);

So it was working against both BigInt and number. Now nodejs v15 assumes the stats field is a BigInt.

3cp commented 3 years ago

BigInt support was added to Nodejs v10.4.0. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/BigInt

3cp commented 3 years ago

It's a missing implementation in mock-fs: (from nodejs doc)

A object provides information about a file. Objects returned from fs.stat(), fs.lstat() and fs.fstat() and their synchronous counterparts are of this type. If bigint in the options passed to those methods is true, the numeric values will be bigint instead of number, and the object will contain additional nanosecond-precision properties suffixed with Ns.

Nodejs v15 expects bigint probably because it internally uses stat API with bigint:true.

That bigint option for stat() APIs was added in nodejs v10.5.0.

v10.5.0 | Accepts an additional options object to specify whether the numeric values returned should be bigint.

julien-f commented 3 years ago

Thank you for this step by step investigation report, it's a model for open source development :+1:

3cp commented 3 years ago

I will try to implement that bigint option for stat API when I got more time.

julien-f commented 3 years ago

No hurry on my side :slightly_smiling_face:

3cp commented 3 years ago

Something unexpected. After implemented the bigint option, nodejs v16 passed on Windows, but still failed on Linux/macOS. It seems nodejs runs different code path for Win and Posix.

Update: I got some missing part.