move: rewrite move to use fs.rename

[manidlou]

This is with the hope to improve move() to handle cross platform compatibility better!

Should resolve #492, #526, #548.

Since OSes are not consistent in error codes (please see my comments in #548), we either have to handle this when errors happen on fs.rename based on the OS that current process running, or check dest before the code reaches fs.rename. IDK, in my mind, checking dest seemed easier both in terms of implementation and also easier to understand when you read the code.

I like the functional approach for isSrcSubdir that @kolgotko introduced in #541. So added here too 😁

Please feel free to leave any feedback! If we think it looks good, I'll apply the corresponding changes to moveSync() as well.

[coveralls]

Coverage increased (+1.2%) to 86.588% when pulling 6765eff on rewrite-move into f0e62c6 on master.

[manidlou]

This test never happens in real scenario even when we were using fs.link because fs.link never throws EISDIR error at least according to link man page, and fs.rename (rename man page) throws EISDIR when dest is an existing directory but src is not a directory.

it('should move folders across devices with EISDIR error', done => {
    const src = `${TEST_DIR}/a-folder`
    const dest = `${TEST_DIR}/a-folder-dest`

    setUpMockFs('EISDIR')

    fse.move(src, dest, err => {
      assert.ifError(err)
      assert.strictEqual(fs.link.callCount, 1)

      fs.readFile(dest + '/another-folder/file3', 'utf8', (err, contents) => {
        const expected = /^knuckles\r?\n$/
        assert.ifError(err)
        assert.ok(contents.match(expected), `${contents} match ${expected}`)

        tearDownMockFs()

        done()
      })
    })
  })

[RyanZim]

@manidlou Are these changes supposed to be non-breaking changes? Does the new code still pass the old tests?

Overall LGTM, haven't tested, but nothing sticks out as bad.

[RyanZim]

Is rimraf used elsewhere, or can we remove it as a devDependency?

[manidlou]

It is used in a few test files, but we definitely can remove it from our devDependencies since rimraf essentially exists within remove. In addition, there are 2 more devDependencies that I believe are not needed and can be removed: read-dir-files and secure-random. We'll take care of them later!

[RyanZim]

Also @manidlou I'd remove the note about the code license; at this point I think it's too divergent to be recognizable.

[manidlou]

@RyanZim this is the test log when I run new code with old tests. The results were the same on linux and windows.

Can't write to device. Skipping move test.


  + move() - prevent moving into itself
    > when source is a file
      ✓ should move the file successfully even when dest parent is 'src/dest'
      ✓ should move the file successfully when dest parent is 'src/src_dest'
      ✓ should move the file successfully when dest parent is 'src/dest_src'
      ✓ should move the file successfully when dest parent is 'src/dest/src'
      ✓ should move the file successfully when dest parent is 'srcsrc/dest'
    > when source is a directory
      ✓ should move the directory successfully when dest is 'src_dest'
      ✓ should move the directory successfully when dest is 'src-dest'
      ✓ should move the directory successfully when dest is 'dest_src'
      ✓ should move the directory successfully when dest is 'src_dest/src'
      ✓ should move the directory successfully when dest is 'src-dest/src'
      ✓ should move the directory successfully when dest is 'dest_src/src'
      ✓ should move the directory successfully when dest is 'src_src/dest'
      ✓ should move the directory successfully when dest is 'src-src/dest'
      ✓ should move the directory successfully when dest is 'srcsrc/dest'
      ✓ should move the directory successfully when dest is 'dest/src'
      ✓ should move the directory successfully when dest is very nested that all its parents need to be created
      ✓ should return error when dest is 'src/dest'
      ✓ should return error when dest is 'src/src_dest'
      ✓ should return error when dest is 'src/dest_src'
      ✓ should return error when dest is 'src/dest/src'

  move
    ✓ should rename a file on the same device
    ✓ should not move a file if source and destination are the same
    ✓ should error if source and destination are the same and source does not exist
    ✓ should not move a directory if source and destination are the same
    1) should not overwrite the destination by default
    2) should not overwrite if overwrite = false
    ✓ should overwrite file if overwrite = true
    ✓ should overwrite the destination directory if overwrite = true
    ✓ should create directory structure by default
    ✓ should work across devices
    ✓ should move folders
    3) should move folders across devices with EISDIR error
    ✓ should overwrite folders across devices
    ✓ should move folders across devices with EXDEV error
    clobber
      ✓ should be an alias for overwrite
    > when trying to move a folder into itself
      ✓ should produce an error
    > when actually trying to move a folder across devices
      > just the folder
        - should move the folder


  33 passing (182ms)
  1 pending
  3 failing

  1) move
       should not overwrite the destination by default:

      Uncaught AssertionError [ERR_ASSERTION]: throw EEXIST
      + expected - actual

      -false
      +true
      
      at fse.move.err (lib/move/__tests__/move.test.js:110:14)
      at checkDest (lib/move/index.js:43:39)
      at fs.stat (lib/move/index.js:100:12)
      at node_modules/graceful-fs/polyfills.js:287:18
      at FSReqWrap.oncomplete (fs.js:167:5)

  2) move
       should not overwrite if overwrite = false:

      Uncaught AssertionError [ERR_ASSERTION]: throw EEXIST
      + expected - actual

      -false
      +true
      
      at fse.move.err (lib/move/__tests__/move.test.js:123:14)
      at checkDest (lib/move/index.js:43:39)
      at fs.stat (lib/move/index.js:100:12)
      at node_modules/graceful-fs/polyfills.js:287:18
      at FSReqWrap.oncomplete (fs.js:167:5)

  3) move
       should move folders across devices with EISDIR error:
     Uncaught 
  Error
      at Timeout.setTimeout [as _onTimeout] (lib/move/__tests__/move.test.js:17:19)

1 and 2 are not passing because fs.rename doesn't throw EEXIST error. It was for fs.link. In the new code, we handle this by checking the dest beforehand. So, these 2 cases still exist in our tests and they pass there; it's just instead of EEXIST error code, we return error with informative message.

About 3, I commented above.

So, my understanding is move still maintains its contract. Please correct me if I am wrong.

[manidlou]

When I said fs.rename doesn't throw EEXIST error, it may throw that error but that's the same as ENOTEMPTY error that is thrown when dest is a nonempty directory. That means, fs.rename doesn't throw EEXIST when dest is an existing file! When dest is an existing file, it still renames it!

We basically care about this when {overwrite: false}. When {overwrite: true}, dest will be overwritten anyway.

[RyanZim]

Just realized I dropped the ball on this one; will try to get some action here.

[RyanZim]

JP prefers to release this as a major in case there’s regressions. How do you feel about delaying release until Node 10 is out in April?

[manidlou]

JP prefers to release this as a major in case there’s regressions. How do you feel about delaying release until Node 10 is out in April?

Agreed. That's totally fine.

[RyanZim]

Merged to branch v6-dev.