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

remove import { tmpName } from 'tmp-promise' dependency #4553

Open
lukasholzer opened this issue Sep 30, 2022 · 6 comments
Open

remove import { tmpName } from 'tmp-promise' dependency #4553

lukasholzer opened this issue Sep 30, 2022 · 6 comments

Comments

@lukasholzer
Copy link
Contributor

This dependency is not needed for creating a temp directory as node can do that:

import { tmpName } from 'tmp-promise'

const path = await tmpName({ template: 'netlify-test-socket-XXXXXX' })

can be converted to:

import { promises as fs } from 'fs'
import { tmpdir } from 'os'
import { join } from 'path'

const path = await fs.mkdtemp(join(tmpdir(), 'netlify-test-socket-'))
@lukasholzer lukasholzer changed the title remove import { tmpName } from 'tmp-promise' dependency remove import { tmpName } from 'tmp-promise' dependency Sep 30, 2022
@lukasholzer lukasholzer added the good first issue Good for newcomers label Sep 30, 2022
@tinfoil-knight
Copy link
Contributor

@lukasholzer tmpName just generates a random file name using some random chars (and checks if they don't already exist) while fs.mkdtemp actually creates the directory. Should we be changing the behaviour here from just creating a valid temp dir name to actually creating the directory?

@lukasholzer
Copy link
Contributor Author

Yea this is combined. We never just create a name without creating the dir. So we should replacing that + creating the dir with fs.mkdtemp

@muditgaur-1009
Copy link

may i work on it

@tinfoil-knight
Copy link
Contributor

may i work on it

Sure. Go ahead @muditgaur-1009. Let me know if you come across any issues while setting things up.

@muditgaur-1009
Copy link

@tinfoil-knight please review i have made the changes

@danez
Copy link
Contributor

danez commented Jun 9, 2023

As this dependency is only used in tests I would prefer to keep it. It has a bunch of convenient functions.

I noticed though that @netlify/build has it listed under dependencies, which is wrong as it is only used in tests. #5064

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants