BREAKING: Use internal fork of make-dir for mkdirs implementation by RyanZim · Pull Request #756 · jprichardson/node-fs-extra · GitHub
Skip to content

BREAKING: Use internal fork of make-dir for mkdirs implementation - #756

Merged
RyanZim merged 7 commits into
masterfrom
ryan/make-dir
Feb 18, 2020
Merged

RyanZim merged 7 commits into
masterfrom
ryan/make-dir

Conversation

@RyanZim

@RyanZim RyanZim commented Feb 6, 2020 •

Copy link
Copy Markdown
Collaborator

Resolves #619

Everything should work similarly to how it did before; except that we no longer return a file path on success (to match fs.mkdir). Also, errors may be different

Breaking Changes:

  • We no longer return a file path on success (to match fs.mkdir)
  • We do not allow creating root on Windows (matches fs.mkdir)
  • Error codes are a bit different

https://github.com/sindresorhus/make-dir

@RyanZim RyanZim added this to the 9.0.0 milestone Feb 6, 2020
@coveralls

coveralls commented Feb 6, 2020 •

Copy link
Copy Markdown

Comment thread lib/mkdirs/make-dir.js
Comment thread lib/mkdirs/make-dir.js Outdated

@jprichardson jprichardson left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Concept ACK

@brody2consult brody2consult left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I would strongly favor keeping the fork of the mkdirs implementation in its own package somewhere. It should be possible to swap this kind of implementation without too much extra junk in the git history.

I did discover an existing mkdirs package that has not been updated for 5 years or so. Maybe time for someone to update or renew it?

@RyanZim

RyanZim commented Feb 13, 2020

Copy link
Copy Markdown
Collaborator Author

@brodybits Our previous code was a fork of an old version of https://www.npmjs.com/package/mkdirp. That package is well-maintained, our fork is much less-so.

@brody2consult

This comment has been minimized.

@RyanZim

This comment has been minimized.

Resolves #619

Everything should work similarly to how it did before; except that
we no longer return a file path on success (to match fs.mkdir).
Also, errors may be different.
- Error codes are different
- Match fs.mkdir behavior on Windows when creating root
@RyanZim

RyanZim commented Feb 14, 2020

Copy link
Copy Markdown
Collaborator Author

Rebased and added new commits fixing issues above.

@RyanZim
RyanZim requested a review from manidlou February 14, 2020 13:33
@RyanZim

RyanZim commented Feb 17, 2020

Copy link
Copy Markdown
Collaborator Author

@JPeer264 JPeer264 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@RyanZim
RyanZim merged commit ab92b24 into master Feb 18, 2020
@RyanZim
RyanZim deleted the ryan/make-dir branch February 18, 2020 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Recursive mkdir coming to Node core

6 participants