loreanvictor / tmplr

Automate Code Scaffolding
MIT License
27 stars 0 forks source link

Temp directories aren't removed if CLI is killed #35

Closed fwextensions closed 2 months ago

fwextensions commented 10 months ago

I'm trying to create a package that you can use like npm create foo to scaffold a project directory using tmplr. To do this, there needs to be a bin key in the package.json that gets run when the create command is executed. I made a really simple node script for the bin.js:

#!/usr/bin/env node
const { execSync } = require("child_process");
const { relative } = require("path");
const command = `npx tmplr use local:${relative(process.cwd(), __dirname)}`;
execSync(command, { stdio: "inherit" });

I haven't published it yet, so testing locally via something like npx ..\create-foo. Getting the relative path from the current directory to where the .tmplr.yml file seemed to be necessary, as I think it was creating the output within the temp directory and then removing it at the end, leaving nothing behind.

Anyway, there's a prompt in the tmplr file and if I kill the command at that point, the local .use-*** temp directory is not cleaned up. If I run a "normal" recipe with npx tmplr, the directory is cleaned up even if I kill the process.

Another thing that happens in this case is that the exists command doesn't seem to find a local directory. So if I run the script from /projects/test/ and there's a local foo/ directory, the if: exists: below doesn't match when outputPath is foo, so the prompt isn't shown and the directory is removed:

  - if:
      exists: '{{ outputPath }}'
    prompt: '"{{ outputPath }}" already exists. Delete it before continuing?'
    choices:
      - Yes
      - No:
          skip: steps

  - remove: '{{ outputPath }}'

I belatedly realized I didn't actually need the bin.js file and could just put an npm command in the bin field. But setting that to npx tmplr use local:. produces the same bugs as above.

It's possible I'm not setting the paths or using the commands correctly here, but there may also be some bug when the npx tmplr command is run from some other package's npm script.

loreanvictor commented 10 months ago

ok I'd need to check these issues. the use-*** folder not getting deleted might be a bug that I'll look into.

on the other issue, can you provide an example of outputPath?

fwextensions commented 10 months ago

These are the steps before the if check above:

  - read: pluginName
    prompt: 'Enter the name of the plugin:'
    default:
      eval: '{{ filesystem.scopedir | Capital Case }}'

  - read: outputDir
    prompt: 'Enter the directory in which to create the plugin:'
    default:
      eval: '{{ pluginName | kebab-case }}'

  - read: outputPath
    eval: '../{{ outputDir }}'

So if you enter a name like My Test, the outputPath will be ../my-test. The .. is necessary because the working directory seems to be set to the temp directory.

The package has been published and can be run as npm create fwidgets@latest. Canceling that when the prompt is shown will leave the temp directory behind.

The code for the create tool is here: https://github.com/fwextensions/fwidgets/tree/main/packages/create-fwidgets

loreanvictor commented 10 months ago

ok so exists (alongside all other path related commands and expressions) basically ignore directories (generally recipes can only see files and not directories). if that is the issue here, it should be fixed using this check instead:

  - if:
      exists: '{{ outputPath }}/**/*'

p.s. I think it would be nice to have exists support check a list instead of just one path at a time either. I will create the corresponding issue.

fwextensions commented 10 months ago

Seems strange that exists ignores directories while remove doesn't, since you may want to warn the user about removing an existing directory before actually doing it.

loreanvictor commented 10 months ago

Seems strange that exists ignores directories while remove doesn't, since you may want to warn the user about removing an existing directory before actually doing it.

Good point. I'll look into the possibility of making exists behave like remove.

loreanvictor commented 10 months ago

so there was a discrepancy between the behaviour of tmplr local:... and degit command, and between tmplr use local:... and use command. The behaviour is now synchronised on 0.3.2 onwards.

however, generally temporary directories are NOT removed when the process is killed. to see this issue in action, check this sandbox.

fixing this issue requires further work as the CLI needs to tap into SIGINT signal and handle things gracefully from there, which is not trivial to get correctly across all platforms. it also requires passing down an environment to all commands so that they can register their cleanup callbacks.

fwextensions commented 10 months ago

Thanks for digging into it. Is there a better way of running tmplr from a node script? Can the node API be used directly?

loreanvictor commented 10 months ago

Thanks for digging into it. Is there a better way of running tmplr from a node script? Can the node API be used directly?

Kind of. It wouldn't solve this particular issue though, but I suspect it is a nice idea to make it easier to run some of tmplr's functionality programmatically.

loreanvictor commented 2 months ago

from v0.3.5, at least in some situations and in some operating systems, temp directories are also cleaned up when the CLI process is killed (for example with SIGINT). note that I wasn't able to write consistent automated tests for this so I couldn't test it in the CI in different environments unfortunately, so if you spot this issue again, please feel-free to reopen this.