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

feat: replace bluebird Promise with native Promise #167

Merged
merged 1 commit into from
Jan 20, 2024

Conversation

cyrdam
Copy link
Contributor

@cyrdam cyrdam commented Dec 14, 2023

Fix: #157

// Initialize a resolved Promise
let result = Promise.resolve();
// Empty array to store the results
const output = [];
Copy link
Owner

Choose a reason for hiding this comment

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

Could you implement this as a reduce please, then you won't need a global (within this fnction) variable

Choose a reason for hiding this comment

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

I think this way is also ok, it is more clear than reduce

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 don't have anything against a reduce version. One thing makes me curious in this case: how it performs in cases of many items in the array. reduce is not the best in performance for loops, but I didn't want to go for the for-loop version. 😄

I tried to make it a little bit more readable than reduce, just like @a-tonchev mentioned.

Anyway, here is a suggestion code for the function:

function mapSeries(array, mapper) {
    // Use the reduce method to chain promises together
    return array.reduce((promise, item, index) => {
        // For each item in the array, add a new promise to the chain
        return promise.then(output => {
            // Apply the mapper function to the current item
            return mapper(item, index, array).then(res => {
                // Push the result of the mapper function to the output array
                output.push(res);
                // Return the output array for the next iteration
                return output;
            });
        });
    // Start with a resolved promise and an empty array
    }, Promise.resolve([]));
}

It needs more comments to understand what exactly is done.

Choose a reason for hiding this comment

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

@cyrdam one more thing - since bluebird is no more used, you can remove it from the package.json, so that we don't install not needed library there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@simonh1000 do you want to have the mentioned version, or the current implementation from the PR?

Copy link
Owner

Choose a reason for hiding this comment

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

Ok, I've read about the reduce perf concerns. they probably don't apply at the scale of an ftp repo (and anyone with a very large directory would probably want the - as yet unimplemented - feature of upload only new files). But your map approach is not a deal breaker, so thank you for your efforts

Copy link
Owner

@simonh1000 simonh1000 left a comment

Choose a reason for hiding this comment

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

see comment about using a reduce

@@ -48,9 +47,13 @@
{
"name": "keyle",
"url": "https://github.com/keyle"
},
{
"name": "cyrdam",
Copy link
Owner

Choose a reason for hiding this comment

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

👍

// Initialize a resolved Promise
let result = Promise.resolve();
// Empty array to store the results
const output = [];
Copy link
Owner

Choose a reason for hiding this comment

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

Ok, I've read about the reduce perf concerns. they probably don't apply at the scale of an ftp repo (and anyone with a very large directory would probably want the - as yet unimplemented - feature of upload only new files). But your map approach is not a deal breaker, so thank you for your efforts

@simonh1000 simonh1000 merged commit 6ecbec4 into simonh1000:master Jan 20, 2024
@cyrdam cyrdam deleted the feature/remove-bluebird branch January 28, 2024 15:13
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.

Remove Bluebird as dependency
3 participants