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

Use make-dir instead of mkdirp #826

Closed
wants to merge 2 commits into from
Closed

Conversation

jonchurch
Copy link
Member

@jonchurch jonchurch commented Jan 17, 2020

This PR is meant to fix #139

This PR replaces mkdirp which doesn't seem to be maintained anymore with make-dir.

I realize that v2 of multer likely won't use storage engines at all, but maybe this can make it into v1.x.

All tests pass, and I don't believe this should be breaking for anyone, as it should function exactly the same as mkdirp did previously. make-dir was created to replace mkdirp.

I also added a test to make sure that a path is created when diskStorage is given a string for destination, as per the docs.

* Write test for checking if dir is created when destination is passed
as string
@jonchurch
Copy link
Member Author

Aww heck, I didn't realize this uses async under the hood, so it will only work on Node >=8 😞

@LinusU
Copy link
Member

LinusU commented Jan 18, 2020

Yeah, unfortunately this would be a breaking change, and thus we cannot add it to 1.x 😬

If you any other library we could use I'd be happy to consider it, but we cannot break compatibility with Node.js 0.10.0.

@jonchurch
Copy link
Member Author

jonchurch commented Jan 18, 2020

I tweeted at the maintainers of mkdirp to see if they'd consider a new release to fix this. Likely won't go anywhere, but figured I'd ask nicely.

I could publish something that fixes this, or we could roll our own, but I don't really think it's worth it.

@wesleytodd
Copy link
Member

wesleytodd commented Jan 21, 2020

@isaacs do you think you could take a look at this? Looks like this is the result of https://github.com/substack/node-mkdirp/issues/70 not being resolved. Is there something we could do on this? Or is the correct move to use make-dir in 1.x and then move to node core fs.mkdir() recursive in 2.x? EDIT: re-read above and see that make-dir is not backwards compatible, so I guess the other option is to deprecate 1.x and get 2 out :P

@isaacs
Copy link

isaacs commented Jan 22, 2020

I think using make-dir is a reasonable approach. I've wanted to take some time to update mkdirp, but other things have taken precedence. I may get to it soon, but then again, I would've said that a few months ago, and here we are. I might also just scrap it and migrate to make-dir myself, because there are just other things too care about. And I believe substack is busy creating peer to peer maps of his tropical volcano island paradise.

@jonchurch
Copy link
Member Author

jonchurch commented Mar 23, 2020

For context, it looks like @isaacs forked mkdirp and published a new major to npm. That version supports >= 8.x same as make-dir. With multer v2 coming out soon, I don't think the linked issue will be fixed for v1.x.

@jonchurch jonchurch closed this Mar 23, 2020
@jonchurch
Copy link
Member Author

jonchurch commented Mar 23, 2020

I opened a PR w/ the fix for the issue this PR was attempting to circumvent on the newly maintained branch for the 0.x line of mkdirp. isaacs/node-mkdirp#15

So there's hope yet!

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.

Invalid dest causes runaway node process
4 participants