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 sync script #13

Merged
merged 10 commits into from
Nov 29, 2021
Merged

Add sync script #13

merged 10 commits into from
Nov 29, 2021

Conversation

marcofugaro
Copy link
Contributor

Sometimes, some packages use just the sync commands of node, no await.
It would be useful if this package offered a sync version as well.

With this PR, you would import it and use it like this:

const fastFolderSizeSync = require('fast-folder-size/sync')

const bytes = fastFolderSizeSync('.')

console.log(bytes)

@simoneb
Copy link
Owner

simoneb commented Nov 28, 2021

I like the idea but the implementation could be improved:

  • the code is pretty much duplicated from the async version, it would be good to find a way to refactor it and remove duplication
  • types are missing
  • tests are missing

@marcofugaro
Copy link
Contributor Author

the code is pretty much duplicated from the async version, it would be good to find a way to refactor it and remove duplication

Any idea of how to do that? I couldn't find a simple solution.

types are missing
tests are missing

I can take care of that.

@simoneb
Copy link
Owner

simoneb commented Nov 28, 2021

the code is pretty much duplicated from the async version, it would be good to find a way to refactor it and remove duplication

Any idea of how to do that? I couldn't find a simple solution.

What changes across the platforms are the arguments provided to the exec* methods and the processing of the output. If we extract those in a map whose key is the platform (with a catch-all option) we will limit the duplication to the strictly necessary to provide the 2 types of sync/async apis.

types are missing
tests are missing

I can take care of that.

Thanks

@marcofugaro
Copy link
Contributor Author

Thanks for the suggestion, all done!

@marcofugaro
Copy link
Contributor Author

Ad damn the sync test is failing on windows..
image

@marcofugaro
Copy link
Contributor Author

All good now, @simoneb could you check?

Copy link
Owner

@simoneb simoneb left a comment

Choose a reason for hiding this comment

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

test.js Outdated Show resolved Hide resolved
@marcofugaro
Copy link
Contributor Author

Can you add type tests in https://github.com/simoneb/fast-folder-size/blob/master/types/index.test-d.ts?

I did, but how do I import fastFolderSizeSync from 'index.d.ts'?

image

@simoneb
Copy link
Owner

simoneb commented Nov 29, 2021

Can you add type tests in https://github.com/simoneb/fast-folder-size/blob/master/types/index.test-d.ts?

I did, but how do I import fastFolderSizeSync from 'index.d.ts'?

image

I don't know, I'm not a ts expert :)

@marcofugaro
Copy link
Contributor Author

Me neither 🤣

@simoneb
Copy link
Owner

simoneb commented Nov 29, 2021

Me neither 🤣

Look up how other packages are doing this perhaps. Anyway, from within the type tests I believe you need to import ./sync, not the full package name

@marcofugaro
Copy link
Contributor Author

Alright, it works all fine now, found a way.

The types I did were based on this stackoverflow answer

Comment on lines +11 to +13
declare module 'fast-folder-size/sync' {
export = fastFolderSizeSync
}
Copy link
Owner

@simoneb simoneb Nov 29, 2021

Choose a reason for hiding this comment

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

despite the tests passing, I have a feeling that defining the types this way will not let a typescript consumer use it in the way you intend it:

import fastFolderSizeSync from 'fast-folder-size/sync'
// or
const fastFolderSizeSync = require('fast-folder-size/sync')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's correct, what are you proposing?

Copy link
Owner

Choose a reason for hiding this comment

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

I'm happy to merge and release a new version. Will you have a chance to try the released package to see if the typings work correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay I will!

@simoneb simoneb merged commit 334b7fb into simoneb:master Nov 29, 2021
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

2 participants