-
Notifications
You must be signed in to change notification settings - Fork 86
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
Conversation
mock-fs works in nodejs v4, but travis windows has trouble to install npm@3 for nodejs v4.
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 |
For those who wants to test this feature, update your package.json with |
@huochunpeng - what a great contribution! Thank you so much for your effort on this. I'll cut a release shortly. |
@tschaub thx for faster-than-expected release! However, can you review item 4 and item 6 of my changes? |
Rather than removing node 4 entirely, you could also make the windows node 4 an allowed failure |
@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 😄 |
- os: windows
node_js: 4 |
Thx, I will give it a try. |
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:
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 runninglib/index.spec.js
in nodejs v10, those fs methods exercised the new code inlib/binding.js
.2. added support of
kUsePromises
andBinding.prototype.openFileHandle
to support experimentalfs.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.
NaN
in file stats uid/gid.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 unpatchedbinding.writeBuffers()
method.Details in
lib/index.js
comments.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.Closes #256, #254, #245, #238
Supersedes #259