raszi / node-tmp

Temporary file and directory creator for node.js
MIT License
736 stars 92 forks source link

Update dependencies to latest versions #198

Closed matsev closed 4 years ago

matsev commented 5 years ago

The rimraf dependency is out of date, and so are some of the devDependencies. This PR updates the dependencies to the latest version.

matsev commented 5 years ago

I get the same failing tests when running the tests on the master branch on my local computer (i.e. without my changes), so I am not convinced that they origin from this PR. My best guess is that they origin from (transitive) dependency updates since node-tmp does not have package-lock.json that set them in stone.

raszi commented 5 years ago

Thank you @matsev for your PR. Unfortunately we cannot simply update the dependencies like that because we are still supporting Node v6.

matsev commented 5 years ago

@raszi Thanks for the feedback. May I suggest that you bump the Node version to v10 which is the current Active LTS release (or at least to Node version v8 which has Maintenance LTS status). v6.9.0 was declared end of live in April this year. Please see the releases page for more information about the Node release schedule.

silkentrance commented 4 years ago

@raszi since a few releases back we only support node >=7. This is due to the fact that SIGINT handlers do not work with node 6 and the respective tests will just fail.

@matsev I have checked out your PR and ran the tests locally. It seems as if this is a race condition. After having run the tests locally against node v12.10.0, I encountered the same issue. However, checking the temporary path tells me that the temporary directory had been removed as expected. Yet, during test evaluation, the temporary directory still exists. I will look into this, maybe we need to introduce some, yikes, wait time before checking whether the directory does no longer exist.

@matsev It is definitely not a race condition. It seems that with v12.10.0 the behaviour changed and graceful cleanup will no longer work.

silkentrance commented 4 years ago

And this happens when you swallow up exceptions, my bad:

TypeError [ERR_INVALID_ARG_TYPE]: The "options" argument must be of type object. Received type function
    at Object.rmdirSync (fs.js:754:13)
    at Array._cleanupCallback (/var/workspaces/01-coldrye/01-collaboration/node-tmp/lib/tmp.js:9:18611)
    at _garbageCollector (/var/workspaces/01-coldrye/01-collaboration/node-tmp/lib/tmp.js:9:19234)
    at process._tmp$safe_listener (/var/workspaces/01-coldrye/01-collaboration/node-tmp/lib/tmp.js:9:23541)
    at process.emit (events.js:214:15)
TypeError [ERR_INVALID_ARG_TYPE]: The "options" argument must be of type object. Received type function
    at Object.rmdirSync (fs.js:754:13)
    at Array._cleanupCallback (/var/workspaces/01-coldrye/01-collaboration/node-tmp/lib/tmp.js:9:18611)
    at _garbageCollector (/var/workspaces/01-coldrye/01-collaboration/node-tmp/lib/tmp.js:9:19234)
    at process._tmp$safe_listener (/var/workspaces/01-coldrye/01-collaboration/node-tmp/lib/tmp.js:9:23541)
    at process.emit (events.js:214:15)
TypeError [ERR_INVALID_ARG_TYPE]: The "options" argument must be of type object. Received type function
    at Object.rmdirSync (fs.js:754:13)
    at Array._cleanupCallback (/var/workspaces/01-coldrye/01-collaboration/node-tmp/lib/tmp.js:9:18611)
    at _garbageCollector (/var/workspaces/01-coldrye/01-collaboration/node-tmp/lib/tmp.js:9:19234)
    at process._tmp$safe_listener (/var/workspaces/01-coldrye/01-collaboration/node-tmp/lib/tmp.js:9:23541)
    at process.emit (events.js:214:15)
TypeError [ERR_INVALID_ARG_TYPE]: The "options" argument must be of type object. Received type function
    at Object.rmdirSync (fs.js:754:13)
    at Array._cleanupCallback (/var/workspaces/01-coldrye/01-collaboration/node-tmp/lib/tmp.js:9:18611)
    at _garbageCollector (/var/workspaces/01-coldrye/01-collaboration/node-tmp/lib/tmp.js:9:19234)
    at process._tmp$safe_listener (/var/workspaces/01-coldrye/01-collaboration/node-tmp/lib/tmp.js:9:23541)
    at process.emit (events.js:214:15)
TypeError [ERR_INVALID_ARG_TYPE]: The "options" argument must be of type object. Received type function
    at Object.rmdirSync (fs.js:754:13)
    at Array._cleanupCallback (/var/workspaces/01-coldrye/01-collaboration/node-tmp/lib/tmp.js:9:18611)
    at _garbageCollector (/var/workspaces/01-coldrye/01-collaboration/node-tmp/lib/tmp.js:9:19234)
    at process._tmp$safe_listener (/var/workspaces/01-coldrye/01-collaboration/node-tmp/lib/tmp.js:9:23541)
    at process.emit (events.js:214:15)
TypeError [ERR_INVALID_ARG_TYPE]: The "options" argument must be of type object. Received type function
    at Object.rmdirSync (fs.js:754:13)
    at Array._cleanupCallback (/var/workspaces/01-coldrye/01-collaboration/node-tmp/lib/tmp.js:9:18611)
    at _garbageCollector (/var/workspaces/01-coldrye/01-collaboration/node-tmp/lib/tmp.js:9:19234)
    at process._tmp$safe_listener (/var/workspaces/01-coldrye/01-collaboration/node-tmp/lib/tmp.js:9:23541)
    at process.emit (events.js:214:15)
silkentrance commented 4 years ago

So we have multiple errors here:

  1. the remove callback will pass the next callback to also rmdirSync, which does not work this way node v12.10.0 or prior introduced options to rmdirSync, namely experimental recursive delete sigh
  2. and there is definitely a race condition somewhere (i had introduced console.log statements in the source and the tests no longer failed, and after having removed these log statements, the tests started failing again)
silkentrance commented 4 years ago

It seems that the behaviour of node fs changed in at least 12.0.x. With the introduction of the experimental option recursive it will no longer remove an empty directory, which seems to be a bug. Still, we must rely on the platform to not delete any non empty directories.

After having changed the rmdirsync callback to rimraf the tests will no longer fail and the empty directories will be removed as expected.

node 12.10.x seems to be broken.

I am still figuring out on how we could achieve this without reimplementation of recursive deletes.

silkentrance commented 4 years ago

The actual failure lies in node v12.10.x which introduced the, seemingly, non optional second argument to fs.rmdirSync, which expects it to be a configuration object.

And while we definitely should fix our software, it stands to reason on why the peops over at node would introduce an optional, yet mandatory, formal parameter... while they could always check for additional parameters via the arguments list.

silkentrance commented 4 years ago

In addition, it seems as if node v12.10.x is now preventing from removing empty directories if the experimental recursive option is missing, again, something that we would refrain on since we need the user to be sure that recursively deleting non empty directories will result in an expected failure.

silkentrance commented 4 years ago

@matsev I have some changes that I would like to include with your PR. Can you please check the appropriate option for the PR?

See https://help.github.com/en/articles/committing-changes-to-a-pull-request-branch-created-from-a-fork for more information.

matsev commented 4 years ago

@silkentrance I guess that you are referring to the Allow edits from maintainers checkbox? As far as I can tell it is checked so you should be good to go.

silkentrance commented 4 years ago

@matsev I have rebased you PR to the latest master. Let's see whether this will build without errors.

silkentrance commented 4 years ago

@matsev build is still failing with latest version of node. need to investigate.

silkentrance commented 4 years ago

@matsev I somehow have misplaced the fix I made for this issue as mentioned in https://github.com/raszi/node-tmp/issues/199#issuecomment-538660745.

Now, which machine has it...

silkentrance commented 4 years ago

Finally I managed to get my head around this. Turned out that I had to actually provide my credentials when pushing to your branch. In the past I thought this to be a mistake of mine. Damn.

silkentrance commented 4 years ago

Working now. Merging now.