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

Use the Node.js 10.12 recursive options when available #5

Closed
wants to merge 1 commit into from
Closed

Use the Node.js 10.12 recursive options when available #5

wants to merge 1 commit into from

Conversation

coderaiser
Copy link

In Node v10.12 a new flag recursive was introduced to fs.mkdir.

Looks like c++ implementation 38% faster then js.
Would be great to have ability to benefit from it using make-dir on node > v10.12 and avoid additional checks and feature detecting for lover node versions :).

@sindresorhus
Copy link
Owner

Tests are failing.

@coderaiser
Copy link
Author

coderaiser commented Oct 12, 2018

They failing on my ubuntu with no modification as well.
When I do:

git clone https://github.com/sindresorhus/make-dir; cd make-dir; npm i; npm test

I see:

Cloning into 'make-dir'...
remote: Enumerating objects: 77, done.
remote: Total 77 (delta 0), reused 0 (delta 0), pack-reused 77
Unpacking objects: 100% (77/77), done.
npm WARN ajv-keywords@3.2.0 requires a peer of ajv@^6.0.0 but none is installed. You must install peer dependencies yourself.
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: fsevents@1.2.4 (node_modules/fsevents):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for fsevents@1.2.4: wanted {"os":"darwin","arch":"any"} (current: {"os":"linux","arch":"x64"})

added 1108 packages from 392 contributors and audited 6419 packages in 34.547s
found 0 vulnerabilities


> make-dir@1.3.0 test /home/coderaiser/make-dir
> xo && nyc ava


  index.js:29:2
  ⚠  29:2  Unexpected todo comment.  no-warning-comments

  1 warning

  17 passed
  2 failed

  async › root dir

  /home/coderaiser/make-dir/test/helpers/util.js:15

   14:   t.true(pathType.dirSync(dir));
   15:   t.is(fs.statSync(dir).mode & 0o777, mode);
   16: };

  Difference:

  - 493
  + 509

  is (test/helpers/util.js:15:4)
  Test.fn (test/async.js:49:2)



  sync › root dir

  /home/coderaiser/make-dir/test/helpers/util.js:15

   14:   t.true(pathType.dirSync(dir));
   15:   t.is(fs.statSync(dir).mode & 0o777, mode);
   16: };

  Difference:

  - 493
  + 509

  is (test/helpers/util.js:15:4)
  Test.t [as fn] (test/sync.js:51:2)
  processEmit [as emit] (node_modules/nyc/node_modules/signal-exit/index.js:155:32)
  processEmit [as emit] (node_modules/nyc/node_modules/signal-exit/index.js:155:32)
----------|----------|----------|----------|----------|-------------------|
File      |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
----------|----------|----------|----------|----------|-------------------|
All files |    95.65 |       90 |      100 |    95.45 |                   |
 index.js |    95.65 |       90 |      100 |    95.45 |             39,66 |
----------|----------|----------|----------|----------|-------------------|
npm ERR! Test failed.  See above for more details.

@sindresorhus
Copy link
Owner

You're right. Hmm, weird. Not sure why master branch is suddenly failing. Something must have changed in recent Node.js patch versions... Any ideas?

@sindresorhus sindresorhus changed the title Added ability to use node v10.12 recursive flag where available Use the Node.js 10.12 recursive options when available Oct 13, 2018
@coderaiser
Copy link
Author

I have no idea. I do not understand what are you checking with this tests. Is it mode of a root dir? But what for, root is exist, and it's mode should not change with make-dir.

@coderaiser
Copy link
Author

Fixed tests, as I understood a problem was with root mode which should be passed directly, because it is not 0o777, but 0o755.

@sindresorhus
Copy link
Owner

Your change makes the test fail on Node.js 8. Something changed in Node.js 10. I'm just not sure exactly what.

@sindresorhus
Copy link
Owner

The tests are fixed in master branch. Can you fix the merge conflict?

@coderaiser
Copy link
Author

Done.

@@ -31,7 +31,10 @@ module.exports = (input, opts) => Promise.resolve().then(() => {
const stat = pify(opts.fs.stat);

const make = pth => {
return mkdir(pth, opts.mode)
return mkdir(pth, {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a problem because someone could set opts.fs containing an alternate implementation mkdir.

I think you can only use mkdir(pth, options_object) only when node.js is at least 10.12 and opts.fs.mkdir === fs.mkdir. In all other cases the legacy mkdir(pth, opts.mode) method needs to be used.

The same comment applies to the sync method, it needs to check the version and verify opts.fs.mkdirSync === fs.mkdirSync before using the options object.

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.

3 participants