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

readFileSync with 'utf-8' option broken by node v20.5.0 #377

Closed
lg250137 opened this issue Jul 25, 2023 · 5 comments
Closed

readFileSync with 'utf-8' option broken by node v20.5.0 #377

lg250137 opened this issue Jul 25, 2023 · 5 comments

Comments

@lg250137
Copy link

Node v20.5.0 introduced an optimization to fs.readFileSync for utf-8 files. Unfortunately, this breaks mock-fs. Here is a minimal test case that works with Node v20.4.0 but breaks with Node v20.5.0:

import mockFs from "mock-fs";
import { test } from "node:test";
import { readFileSync } from "fs";
import assert from "node:assert/strict";

test("mockFs", () => {
    mockFs({ "file": "contents" })
    const fileContents = readFileSync("file", 'utf-8').toString()
    assert.equal(fileContents, "contents")
})

In Node v20.5.0, I get the error Error: ENOENT: no such file or directory, open 'file'.
The above test case also works in Node v20.5.0 if I remove the 'utf-8' option from readFileSync. This issue specifically occurs for readFileSync with 'utf-8' option.

Here is the relevant commit: nodejs/node#48658

@brendandburns
Copy link

fwiw, anyone who hits this, you can replace fs.readFileSync(foo, 'utf-8') with fs.readFileSync(foo).toString('utf-8') and it will fix the mocking.

Of course you will lose the performance improvements in the nodejs change, so it's probably worth fixing mock-fs.

corymhall added a commit to aws/jsii-rosetta that referenced this issue Aug 17, 2023
Implementing the workaround mentioned in the linked issue.

re tschaub/mock-fs#377
github-merge-queue bot pushed a commit to aws/jsii-rosetta that referenced this issue Aug 17, 2023
Implementing the workaround mentioned in the linked issue. The only
downside of this change is losing out on the performance improvements in
v20.5.

re tschaub/mock-fs#377



---

By submitting this pull request, I confirm that my contribution is made
under the terms of the [Apache 2.0 license].

[Apache 2.0 license]: https://www.apache.org/licenses/LICENSE-2.0
@lg250137
Copy link
Author

fwiw, anyone who hits this, you can replace fs.readFileSync(foo, 'utf-8') with fs.readFileSync(foo).toString('utf-8') and it will fix the mocking.

Of course you will lose the performance improvements in the nodejs change, so it's probably worth fixing mock-fs.

Unfortunately in my case I depend on a library that uses fs.readFileSync(foo, 'utf-8'), so I can't use that workaround.

chadlwilson added a commit to getgauge/gauge-js that referenced this issue Sep 23, 2023
chadlwilson added a commit to getgauge/gauge-js that referenced this issue Sep 25, 2023
chadlwilson added a commit to getgauge/gauge-js that referenced this issue Sep 26, 2023
sriv pushed a commit to getgauge/gauge-js that referenced this issue Sep 26, 2023
* Use canonical encoding/readFile command to take advantage of fast path in Node 20

Fast path doesn't appear to check encoding options case insensitive, 'utf8' seems more canonical.

Signed-off-by: Chad Wilson <[email protected]>

* Apply workaround for mock-fs Node 20 issue

See tschaub/mock-fs#377

Signed-off-by: Chad Wilson <[email protected]>

* Migrate build to Node 20

Node 16 is nearly EOL, and Node 20 is nearly LTS, so let's go straight there. This remove use of a deprecated npm flag, replacing with alternative compatible with npm 7/NodeJS 16 onwards (2021+, Node 16 recently went EOL in Sep 2023). The package-lock is also now only compatible with Node 16/npm 7 onwards.

Since this is technically a breaking change in npm/node compatibility, bumps the major version and updates the documentation as such.

Signed-off-by: Chad Wilson <[email protected]>

---------

Signed-off-by: Chad Wilson <[email protected]>
RythenGlyth added a commit to RythenGlyth/advanced-ftp that referenced this issue Oct 16, 2023
stennie added a commit to stennie/repoman that referenced this issue Oct 30, 2023
basti1302 pushed a commit to basti1302/repoman that referenced this issue Oct 30, 2023
* Fix #57: Bump supported Node.js engine from 4.0.0 to 18.0.0

* Test & update some devDependencies

* Fix #58: Remove Gitorious features

* Restore missing close parens

* Fix #56: Deprecated package warnings

Note: some outstanding warnings due to cliffano/bagofrequest#6

* Update Node.js versions to test against

* Use 20.4 as the max test version since mock-fs currently fails on higher versions
(possible related to tschaub/mock-fs#377)

* Remove unused file with output of `npm ls --all`

* Update sinon & mock-fs versions
basti1302 pushed a commit to basti1302/repoman that referenced this issue Nov 9, 2023
* Fix #57: Bump supported Node.js engine from 4.0.0 to 18.0.0

* Test & update some devDependencies

* Fix #58: Remove Gitorious features

* Restore missing close parens

* Fix #56: Deprecated package warnings

Note: some outstanding warnings due to cliffano/bagofrequest#6

* Update Node.js versions to test against

* Use 20.4 as the max test version since mock-fs currently fails on higher versions
(possible related to tschaub/mock-fs#377)

* Remove unused file with output of `npm ls --all`

* Update sinon & mock-fs versions
hegemonic added a commit to jsdoc/jsdoc that referenced this issue Dec 3, 2023
Workaround for tschaub/mock-fs#377, which causes `fs.readFileSync()` to fail on Node.js >=20.5.0.

Fixes #2097.
hegemonic added a commit to hegemonic/jsdoc-baseline that referenced this issue Dec 19, 2023
Also removes `fs-extra`, which seems less necessary now than it did back when I added it, and updates a bunch of stuff to use async I/O.

If we update all of the output-generating tests to put the output in a temp directory, then I think we could remove `mock-fs` entirely.
hegemonic added a commit to hegemonic/jsdoc-baseline that referenced this issue Dec 20, 2023
It has some nasty, hard-to-fix bugs in current versions of Node.js (tschaub/mock-fs#377, tschaub/mock-fs#380) and an uncertain future (tschaub/mock-fs#383).

Also, this package's tests don't really need to mock the entire filesystem; they just need to write files to a temp directory that can be deleted when the tests are complete.
@marikaner
Copy link

I guess this PR relates to this as well: https://github.com/tschaub/mock-fs/pull/381/files.

@cihandeniz
Copy link

readFileSync('...').toString('utf8') was an ok workaround for me, but I had issues with existsSync and writeFileSync as well. I'm posting my temp solution in case anyone else has the same problem.

// fsExt.js
import fs from "fs";

function existsSync(path) {
  try {
    fs.statSync(path);
    return true;
  } catch (error) {
    if(error.code === "ENOENT") {
      return false;
    } else {
      throw error;
    }
  }
}

function readFileSync(path, encoding) {
  return fs.readFileSync(path).toString(encoding);
}

function writeFileSync(path, data) {
  const stream = fs.openSync(path, "w");
  fs.writeSync(stream, data, 0, "utf8");
  fs.closeSync(stream);
}

export {
  existsSync,
  readFileSync,
  writeFileSync
};
// elsewhere in your code where you need above file operations

// import { existsSync, writeFileSync, readFileSync } from "fs";
import { existsSync, writeFileSync, readFileSync } from "./fsExt.js";

...

p.s. above code doesn't fully wrap fs functions, it was enough in my case.

@tschaub
Copy link
Owner

tschaub commented Sep 26, 2024

@lg250137 - I added a test based on your example in 9c87858

This is passing now - likely fixed by #387.

Feel free to reopen if a more specific test cases fails.

@tschaub tschaub closed this as completed Sep 26, 2024
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

No branches or pull requests

5 participants