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

feat: support nodejs v10 and v11, support experimental fs.promises #260

Merged
merged 7 commits into from
Feb 4, 2019
Merged

Conversation

3cp
Copy link
Collaborator

@3cp 3cp commented Feb 4, 2019

I was terrified by the amount of refactor work in my own projects to remove mock-fs. So I jump in to fix mock-fs instead.

First, it passes all tests in nodejs v4, v6, v8, v10, v11 on Linux/Windows/macOS.
https://travis-ci.org/huochunpeng/mock-fs/builds/488264371

Note:

  1. the failure of nodejs v10 and v11 on windows is probably due to a bug in nodejs itself. on Win10, fs.stat() returns a stats with undefined blksize and blocks nodejs/node#25913
  2. removed v4 from travis-ci matrix. v4 works fine, but travis-ci windows box just had problem to install npm@3. BTW nodejs v4 is EOL anyway.

Tested on one of the projects I am working on, it finally can pass tests in nodejs v10. https://travis-ci.org/huochunpeng/cli/builds/488267193

What's in this PR:

1. nodejs v10 fs binding has added one more optional parameter ctx to all methods.

It is used to capture the error details when calling fs synced methods. I am too lazy to add the test coverage in lib/binding.spec.js. But all the new code is covered when running lib/index.spec.js in nodejs v10, those fs methods exercised the new code in lib/binding.js.

2. added support of kUsePromises and Binding.prototype.openFileHandle to support experimental fs.promises.

I did not add enough test coverage to this yet. Only got one on readFile. I need to read more on fs.promises, then add more test coverage, probably in another PR.

3. replaced hard coded uv error codes with the runtime errmap in uv binding.

Windows has different errno numbers than posix os.

4. normalized uid/gid to NaN on OS (Windows) does support it.

This fixed some failing tests on Windows.
I am not sure this is the correct thing.

  • The existing tests here expect NaN in file stats uid/gid.
  • On windows, nodejs v10 or v8 (without mock-fs), uid/git are 0 for any valid file stats. I didn't test v6 and v4.

5. Disable file write stream's _writev method.

The nodejs v10+ _writev implementation uses unpatched binding.writeBuffers() method.

Details in lib/index.js comments.

// Have to disable write stream _writev on nodejs v10+.
//
// nodejs v8 lib/fs.js
// note binding.writeBuffers will use mock-fs patched writeBuffers.
//
//   const binding = process.binding('fs');
//   function writev(fd, chunks, position, callback) {
//     // ...
//     binding.writeBuffers(fd, chunks, position, req);
//   }
//
// nodejs v10+ lib/internal/fs/streams.js
// note it uses original writeBuffers, bypassed mock-fs patched writeBuffers.
//
//  const {writeBuffers} = internalBinding('fs');
//  function writev(fd, chunks, position, callback) {
//    // ...
//    writeBuffers(fd, chunks, position, req);
//  }
//
// Luckily _writev is an optional method on Writeable stream implementation.
// When _writev is missing, it will fall back to make multiple _write calls.

6. weird fix for read in nodejs v10+.

The read seems suffered similar issue (calling unpatched binding method) as the write stream issue solved in (5).
#254 (comment)

When I solved it, I have not read through the above issue. I don't understand what's going on, but anyway, my fix works.

Details in lib/binding.js comments.

// I don't know exactly what is going on.
// If _openFiles is a property of binding instance, there is a strange
// bug in nodejs v10+ that something cleaned up this._openFiles from
// nowhere. It happens after second mockfs(), after first mockfs()+restore().

// So I moved _openFiles to a private var. The other two vars (_system,
// _counter) do not hurt.
// This fixed https://github.com/tschaub/mock-fs/issues/254
// But I did not dig deep enough to understand what exactly happened.
var _system;
var _openFiles = {};
var _counter = 0;

Closes #256, #254, #245, #238
Supersedes #259

@3cp
Copy link
Collaborator Author

3cp commented Feb 4, 2019

The ci check here shows exactly same result as my own travis run. The failure is likely due a nodejs v10 bug on windows. nodejs/node#25913

@3cp
Copy link
Collaborator Author

3cp commented Feb 4, 2019

For those who wants to test this feature, update your package.json with "mock-fs": "huochunpeng/mock-fs#node10". Thx!

@tschaub
Copy link
Owner

tschaub commented Feb 4, 2019

@huochunpeng - what a great contribution! Thank you so much for your effort on this. I'll cut a release shortly.

@3cp
Copy link
Collaborator Author

3cp commented Feb 4, 2019

@tschaub thx for faster-than-expected release!

However, can you review item 4 and item 6 of my changes?

@3cp 3cp deleted the node10 branch February 4, 2019 20:40
@ljharb
Copy link

ljharb commented Feb 5, 2019

Rather than removing node 4 entirely, you could also make the windows node 4 an allowed failure

@3cp
Copy link
Collaborator Author

3cp commented Feb 5, 2019

@ljharb I did try reading allow_failures for maybe 2 mins, did not get an obvious answer on how to target both nodejs and os within my patient, so I lazily gave up 😄

@ljharb
Copy link

ljharb commented Feb 5, 2019

- os: windows
  node_js: 4

@3cp
Copy link
Collaborator Author

3cp commented Feb 5, 2019

Thx, I will give it a try.

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

Successfully merging this pull request may close these issues.

existsSync throws an error instead of returning false in Node 11
3 participants