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 walk() and walkSync() #338

Closed
jprichardson opened this issue Jan 2, 2017 · 14 comments · Fixed by #343
Closed

Remove walk() and walkSync() #338

jprichardson opened this issue Jan 2, 2017 · 14 comments · Fixed by #343
Milestone

Comments

@jprichardson
Copy link
Owner

jprichardson commented Jan 2, 2017

@RyanZim and I have decided to remove walk() and walkSync().

This was not an easy decision, but the long-term vision of this library is to provide Node.js programmers with an easy to use, batteries-included way to do file system operations. While we've succeeded up to this point, @RyanZim and I must be judicious about our resources.

I think there's a strong case to be made for walk() to be in the library, but the Streams interface may not belong - perhaps generators are a better way to accomplish this?

ES2015 and ES2017 bring generators and async/await. This ushers in a new era of writing JavaScript code. I believe that we'll see a decline in using sync() methods over the years. However, before too many people establish dependencies upon the synchronous code, it should be removed sooner rather than later. We will start with walkSync(). The other sync methods will stay with us for awhile (at least one year).

walkSync() joined fs-extra for the 1.0.0 release (two months ago) and with 2.0.0 coming soon, it's going to be removed along with walk(). Users of walk()have a drop-in replacement with klaw. But we don't want to leave users of walkSync() in the dust (I don't think there are many at this point), but @mawni / @agnivade would you be interested in creating a klaw-sync or walk-sync library for us to point to users?

@RyanZim RyanZim added this to the 2.0.0 milestone Jan 2, 2017
@manidlou
Copy link
Collaborator

manidlou commented Jan 2, 2017

That sounds reasonable. I am interested in creating a klaw-sync or walk-sync library. However, since @agnivade is the original auther of walkSync, that'd be great to know what he thinks about it.

@agnivade
Copy link
Contributor

agnivade commented Jan 3, 2017

I can understand about removing the sync functions, but why is walk being removed ? If the long-term vision is to allow users

with an easy to use, batteries-included way to do file system operations.

why move walk into a separate library ?

To me, it doesn't make sense to import a library just to use a walk operation. I would just copy-paste a SO answer and be done with it.

Anyways, that was my 2 cents. I am currently involved in other projects too, which will take my time. So I won't be able to actively spend my time in creating/maintaining a library. @mawni - please go ahead and create it. I would be happy to review it if you would like.

@RyanZim
Copy link
Collaborator

RyanZim commented Jan 3, 2017

@agnivade walk is currently contained in https://github.com/jprichardson/node-klaw. fs-extra simply requires it and exports it as fs.walk().

We hope that in the future, fs-extra will have a walk method; however, (as @jprichardson said) we are not sure if klaw's readable stream interface is the best way to do this moving forward. We would like to reevaluate our approach and allow klaw to stabilize a bit more before re-exposing it as part of fs-extra's public API.

Also, as @jprichardson said, we must be judicious about our resources. Currently, @jprichardson & I have very little time to work on klaw. I would like to focus on cleaning up copy (it currently contains quite a bit of legacy code) and adding support for promises to fs-extra. Cutting out walk for the time being will hopefully allow us to focus on these issues/features.

Anyhow, thanks for the quick response @agnivade.


@mawni Go ahead and extract walkSync; when you have it in its own repo, please comment here. I would like to link to it from the README and/or the release notes for v2. Thanks in advance!

@RyanZim
Copy link
Collaborator

RyanZim commented Jan 3, 2017

Actual removal done on the v2 branch (master) via #339; going to leave this open to track walkSync progress.

@manidlou
Copy link
Collaborator

manidlou commented Jan 4, 2017

Thanks @RyanZim. I will comment here as soon as the module is ready.

@manidlou
Copy link
Collaborator

manidlou commented Jan 9, 2017

@jprichardson, @RyanZim, @agnivade would you please look at https://github.com/mawni/node-klaw-sync? I'd like to know your ideas about it before publishing it.

@agnivade
Copy link
Contributor

agnivade commented Jan 9, 2017

Sure, give me some time though.

@RyanZim
Copy link
Collaborator

RyanZim commented Jan 9, 2017

@mawni LGTM, though I haven't actually tried it. (I never used walk or walkSync personally)

PR welcome here and at klaw linking to your library.

@agnivade
Copy link
Contributor

LGTM overall. Couple of minor points -

  1. Your minimum node version support seem to be 4. So, it would be good to use the latest ES6 standard. Keeps the code cleaner and easy to maintain.

  2. Please add a .eslintrc file and run the linter on travis.

@jprichardson
Copy link
Owner Author

@mawni - looks good to me!

Please add a .eslintrc file and run the linter on travis.

It's already there :p standard (which uses eslint) is being called on npm test.

@agnivade
Copy link
Contributor

Ah I see. In that case, I would suggest creating an extra "lint" section in package.json so that developers can run the linter separately through "npm run lint". Its just a small thing that gives extra flexibility.

@manidlou
Copy link
Collaborator

@jprichardson, @RyanZim, @agnivade thanks a lot. I truly appreciate it.

I would suggest creating an extra "lint" section in package.json so that developers can run the linter separately through "npm run lint".

Great point. For sure, I will add it to package.json.

@RyanZim I will add the klaw-sync link to docs, then submit PR.

@RyanZim
Copy link
Collaborator

RyanZim commented Jan 12, 2017

SGTM

@RyanZim
Copy link
Collaborator

RyanZim commented Jan 13, 2017

Fully resolved with #343. @mawni PR for klaw welcome anytime.

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

Successfully merging a pull request may close this issue.

4 participants