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

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

Open
fwextensions opened this issue Dec 27, 2023 · 8 comments
Open

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

fwextensions opened this issue Dec 27, 2023 · 8 comments
Labels
bug Something isn't working

Comments

@fwextensions
Copy link

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.

@fwextensions fwextensions changed the title Temp directory doesn't get auto-removed when tmplr is run from a package bin script Temp directory doesn't get auto-removed and exists fails when tmplr is run from a package bin script Dec 27, 2023
@loreanvictor
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

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
Copy link
Author

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.

@fwextensions fwextensions changed the title Temp directory doesn't get auto-removed and exists fails when tmplr is run from a package bin script Temp directory doesn't get auto-removed when tmplr is run from a package bin script Dec 28, 2023
@loreanvictor
Copy link
Owner

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 loreanvictor added the bug Something isn't working label Dec 30, 2023
@loreanvictor loreanvictor changed the title Temp directory doesn't get auto-removed when tmplr is run from a package bin script Temp directories aren't removed if CLI is killed Jan 5, 2024
@loreanvictor
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants