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

add concat-stream to devDependencies #353

Merged
merged 2 commits into from Jun 8, 2022

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Jun 8, 2022

Currently, rimraf and concat-stream are imported because they are dependencies of other packages. This PR adds them explicitly.

Copy link
Member

@climba03003 climba03003 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Can we replace rimraf with fs.rmdirSync ?

@kibertoad
Copy link
Member

@climba03003 I doubt it, if they are transitive dependencies, unless we send PRs upstream.
@Uzlopak do you know which packages depend on rimraf?

@climba03003
Copy link
Member

climba03003 commented Jun 8, 2022

I doubt it, if they are transitive dependencies, unless we send PRs upstream.

I means it is added in devDepenencies because we directly using it.
If we remove the direct usage, then it can be removed in devDepenencies.

It only used in one place inside the codebase.

rimraf.sync(tmpdir)

@climba03003 climba03003 changed the title add explicitly rimraf and concat-stream to depDependencies add explicitly rimraf and concat-stream to devDependencies Jun 8, 2022
@kibertoad
Copy link
Member

ah, you are right.
that would be a better solution indeed

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Jun 8, 2022

@kibertoad
tap requires rimraf, as it is a dependency of nyc.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Jun 8, 2022

We could drop rimraf as devDependency by doing

    if (fs.promises.rm) {
       await fs.promises.rm(tmpdir, { recursive: true, force: true })
     } else {
       await fs.promises.rmdir(tmpdir, { recursive: true })
     }

or

await fs.promises.rm(tmpdir, { recursive: true, force: true })

fs.promises.rm is available since 14.14.0

What do you prefer?

@climba03003
Copy link
Member

climba03003 commented Jun 8, 2022

What do you prefer?

await fs.promises.rm(tmpdir, { recursive: true, force: true })

Since we test against the latest version of node (inside that major).
If all of them provide fs.promises.rm, then no need an extra check for it.

@Uzlopak Uzlopak changed the title add explicitly rimraf and concat-stream to devDependencies add concat-stream to devDependencies Jun 8, 2022
@Uzlopak
Copy link
Contributor Author

Uzlopak commented Jun 8, 2022

@climba03003 done ;)

Copy link
Member

@climba03003 climba03003 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@kibertoad kibertoad merged commit 8784672 into fastify:master Jun 8, 2022
@Uzlopak Uzlopak deleted the fix-dependencies branch June 8, 2022 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants