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

Await all promises in the same time in method getEligibleShippingMethods in ShippingCalculator #397

Closed
jonyw4 opened this issue Jul 3, 2020 · 4 comments
Assignees
Milestone

Comments

@jonyw4
Copy link
Contributor

jonyw4 commented Jul 3, 2020

I have some plugins using ShippingCalculator that are promises. Would be nice to all promises were loaded at the same time. Currently, the method getEligibleShippingMethods awaits the response from the shippingMethods individually.

async getEligibleShippingMethods(
ctx: RequestContext,
order: Order,
): Promise<Array<{ method: ShippingMethod; result: ShippingCalculationResult }>> {
const shippingMethods = this.shippingMethodService.getActiveShippingMethods(ctx.channel);
const eligibleMethods: Array<{ method: ShippingMethod; result: ShippingCalculationResult }> = [];
for (const method of shippingMethods) {
const eligible = await method.test(order);
if (eligible) {
const result = await method.apply(order);
if (result) {
eligibleMethods.push({ method, result });
}
}
}
return eligibleMethods.sort((a, b) => a.result.price - b.result.price);

A solution would be using Promises.all.

@jonyw4 jonyw4 changed the title Await all promises in the same time with ShippingCalculator Await all promises in the same time in method getEligibleShippingMethods in ShippingCalculator Jul 3, 2020
@michaelbromley
Copy link
Member

Good point, we can parallelize both the test and the apply steps here.

@jonyw4
Copy link
Contributor Author

jonyw4 commented Jul 4, 2020

I am planning to do a pull request for this issue. Do you have any points before I start to develop?

@michaelbromley
Copy link
Member

No, go ahead and make a PR! 👍

@jonyw4
Copy link
Contributor Author

jonyw4 commented Jul 7, 2020

Good point, we can parallelize both the test and the apply steps here.

To solve the problem I created checkEligibilityByShippingMethod. This function waits for test and apply. So in getEligibleShippingMethods we can make an array of many checkEligibilityByShippingMethod, and then create a Promise.all with everything at the same time.
How apply depends on the response from test I put these two in the same function and apply await for test.

https://github.com/vendure-ecommerce/vendure/pull/407/files#diff-cbb33e9a4b6e3276c9cbe802f7161e80

michaelbromley pushed a commit that referenced this issue Jul 9, 2020
* feat(core): Accept empty as response in shipping calculator

* feat(core): Await all promises at same time in ShippingCalculator

* refactor(core): Update filter undefined shipping calculator

Closes #397, closes #398
@michaelbromley michaelbromley added this to the v0.14.0 milestone Jul 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants