-
Notifications
You must be signed in to change notification settings - Fork 6.8k
perf(cdk/testing): reduce change detections when running parallel actions #21071
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
Conversation
98e78d7
to
33df7f9
Compare
Caretaker note: these changes touch pretty much all test harnesses so it's possible that something could break during the presubmit. If something does break, I'll split this up into individual PRs. |
…ions Switches all of our `testing` code to use `parallel` instead of `Promise.all` which reduces the number of change detections that we'll trigger during tests. Also adds more type overloads to `parallel`, because the previous types didn't allow the `values` function to return mixed value arrays which we had in ~15 instances. I've tried to reduce the amount of code by only implementing up to 5 overloads, but we may want to expand it to 9 like `Promise.all`.
33df7f9
to
36133df
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* @param values A getter for the async values to resolve in parallel with batched change detection. | ||
* @return The resolved values. | ||
*/ | ||
export function parallel<T1, T2, T3, T4, T5>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh good catch, is there some way to do this without having a separate declaration for each number of params?
This is what I get for putting off actually using the function in our own tests 😞
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There isn't. I took this from TS's built-in type for Promise.all
and I've seen rxjs do something similar.
Changed to minor due to the new method signatures |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Switches all of our
testing
code to useparallel
instead ofPromise.all
which reduces the number of change detections that we'll trigger during tests.Also adds more type overloads to
parallel
, because the previous types didn't allow thevalues
function to return mixed value arrays which we had in ~15 instances. I've tried to reduce the amount of code by only implementing up to 5 overloads, but we may want to expand it to 9 likePromise.all
.